diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 944b1b320..d33747d5d 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -30,11 +30,9 @@ from app.models import ( SMS_TYPE, ) from app.notifications.process_notifications import persist_notification -from app.notifications.validators import check_service_over_daily_message_limit from app.serialised_models import SerialisedService, SerialisedTemplate from app.service.utils import service_allowed_to_send_to from app.utils import DATETIME_FORMAT -from app.v2.errors import TooManyRequestsError @notify_celery.task(name="process-job") @@ -59,9 +57,6 @@ def process_job(job_id, sender_id=None): "Job {} has been cancelled, service {} is inactive".format(job_id, service.id)) return - if __sending_limits_for_job_exceeded(service, job, job_id): - return - recipient_csv, template, sender_id = get_recipient_csv_and_template_and_sender_id(job) current_app.logger.info("Starting job {} processing {} notifications".format(job_id, job.notification_count)) @@ -134,24 +129,6 @@ def process_row(row, template, job, service, sender_id=None): return notification_id -def __sending_limits_for_job_exceeded(service, job, job_id): - try: - total_sent = check_service_over_daily_message_limit(KEY_TYPE_NORMAL, service) - if total_sent + job.notification_count > service.message_limit: - raise TooManyRequestsError(service.message_limit) - else: - return False - except TooManyRequestsError: - job.job_status = 'sending limits exceeded' - job.processing_finished = datetime.utcnow() - dao_update_job(job) - current_app.logger.info( - "Job {} size {} error. Sending limits {} exceeded".format( - job_id, job.notification_count, service.message_limit) - ) - return True - - @notify_celery.task(bind=True, name="save-sms", max_retries=5, default_retry_delay=300) def save_sms(self, service_id, diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 101855e93..317deb607 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -138,21 +138,7 @@ def persist_notification( dao_create_notification(notification) if key_type != KEY_TYPE_TEST and current_app.config['REDIS_ENABLED']: current_app.logger.info('Redis enabled, querying cache key for service id: {}'.format(service.id)) - cache_key = redis.daily_limit_cache_key(service.id) total_key = redis.daily_total_cache_key() - current_app.logger.info('Redis daily limit cache key: {}'.format(cache_key)) - if redis_store.get(cache_key) is None: - current_app.logger.info('Redis daily limit cache key does not exist') - # if cache does not exist set the cache to 1 with an expiry of 24 hours, - # The cache should be set by the time we create the notification - # but in case it is this will make sure the expiry is set to 24 hours, - # where if we let the incr method create the cache it will be set a ttl. - redis_store.set(cache_key, 1, ex=86400) - current_app.logger.info('Set redis daily limit cache key to 1') - else: - current_app.logger.info('Redis daily limit cache key does exist') - redis_store.incr(cache_key) - current_app.logger.info('Redis daily limit cache key has been incremented') if redis_store.get(total_key) is None: current_app.logger.info('Redis daily total cache key does not exist') redis_store.set(total_key, 1, ex=86400) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 262dfbe4d..c3e2b0b00 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -2,7 +2,6 @@ from flask import current_app from gds_metrics.metrics import Histogram from notifications_utils import SMS_CHAR_COUNT_LIMIT from notifications_utils.clients.redis import ( - daily_limit_cache_key, daily_total_cache_key, rate_limit_cache_key, ) @@ -30,12 +29,7 @@ from app.notifications.process_notifications import ( 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.v2.errors import ( - BadRequestError, - RateLimitError, - TooManyRequestsError, - TotalRequestsError, -) +from app.v2.errors import BadRequestError, RateLimitError, TotalRequestsError REDIS_EXCEEDED_RATE_LIMIT_DURATION_SECONDS = Histogram( 'redis_exceeded_rate_limit_duration_seconds', @@ -54,27 +48,7 @@ def check_service_over_api_rate_limit(service, api_key): raise RateLimitError(rate_limit, interval, api_key.key_type) -def check_service_over_daily_message_limit(key_type, service): - if key_type == KEY_TYPE_TEST or not current_app.config['REDIS_ENABLED']: - return 0 - - cache_key = daily_limit_cache_key(service.id) - service_stats = redis_store.get(cache_key) - 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) - return service_stats - if int(service_stats) >= service.message_limit: - current_app.logger.info( - "service {} has been rate limited for daily use sent {} limit {}".format( - service.id, int(service_stats), service.message_limit) - ) - raise TooManyRequestsError(service.message_limit) - return int(service_stats) - - -def check_application_over_daily_message_total(key_type, service): +def check_application_over_retention_limit(key_type, service): if key_type == KEY_TYPE_TEST or not current_app.config['REDIS_ENABLED']: return 0 @@ -97,8 +71,7 @@ def check_application_over_daily_message_total(key_type, service): def check_rate_limiting(service, api_key): check_service_over_api_rate_limit(service, api_key) - check_application_over_daily_message_total(api_key.key_type, service) - check_service_over_daily_message_limit(api_key.key_type, service) + check_application_over_retention_limit(api_key.key_type, service) def check_template_is_for_notification_type(notification_type, template_type): diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 9f7f7685b..465e3eac7 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -12,7 +12,6 @@ from app.notifications.process_notifications import ( send_notification_to_queue, ) from app.notifications.validators import ( - check_service_over_daily_message_limit, validate_and_format_recipient, validate_template, ) @@ -45,8 +44,6 @@ def send_one_off_notification(service_id, post_data): validate_template(template.id, personalisation, service, template.template_type) - check_service_over_daily_message_limit(KEY_TYPE_NORMAL, service) - validate_and_format_recipient( send_to=post_data['to'], key_type=KEY_TYPE_NORMAL, diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 65b9c2335..ff5a48e52 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -45,7 +45,6 @@ from app.models import ( ) from app.serialised_models import SerialisedService, SerialisedTemplate from app.utils import DATETIME_FORMAT -from app.v2.errors import TooManyRequestsError from tests.app import load_example_csv from tests.app.db import ( create_api_key, @@ -150,44 +149,6 @@ def test_should_not_process_job_if_already_pending(sample_template, mocker): assert tasks.process_row.called is False -def test_should_not_process_if_send_limit_is_exceeded( - notify_api, notify_db_session, mocker -): - service = create_service(message_limit=9) - template = create_template(service=service) - job = create_job(template=template, notification_count=10, original_file_name='multiple_sms.csv') - mocker.patch('app.celery.tasks.s3.get_job_and_metadata_from_s3', - return_value=(load_example_csv('multiple_sms'), {'sender_id': None})) - mocker.patch('app.celery.tasks.process_row') - mocker.patch('app.celery.tasks.check_service_over_daily_message_limit', - side_effect=TooManyRequestsError("exceeded limit")) - process_job(job.id) - - job = jobs_dao.dao_get_job_by_id(job.id) - assert job.job_status == 'sending limits exceeded' - assert s3.get_job_and_metadata_from_s3.called is False - assert tasks.process_row.called is False - - -def test_should_not_process_if_send_limit_is_exceeded_by_job_notification_count( - notify_api, notify_db_session, mocker -): - service = create_service(message_limit=9) - template = create_template(service=service) - job = create_job(template=template, notification_count=10, original_file_name='multiple_sms.csv') - mock_s3 = mocker.patch('app.celery.tasks.s3.get_job_and_metadata_from_s3', - return_value=(load_example_csv('multiple_sms'), {'sender_id': None})) - mock_process_row = mocker.patch('app.celery.tasks.process_row') - mocker.patch('app.celery.tasks.check_service_over_daily_message_limit', - return_value=0) - process_job(job.id) - - job = jobs_dao.dao_get_job_by_id(job.id) - assert job.job_status == 'sending limits exceeded' - mock_s3.assert_not_called() - mock_process_row.assert_not_called() - - def test_should_process_job_if_send_limits_are_not_exceeded( notify_api, notify_db_session, mocker ): @@ -200,7 +161,6 @@ def test_should_process_job_if_send_limits_are_not_exceeded( mocker.patch('app.celery.tasks.save_email.apply_async') mocker.patch('app.encryption.encrypt', return_value="something_encrypted") mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") - mocker.patch('app.celery.tasks.check_service_over_daily_message_limit', return_value=0) process_job(job.id) s3.get_job_and_metadata_from_s3.assert_called_once_with( diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 4246ff705..7cb54bac3 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -218,9 +218,9 @@ def test_persist_notification_increments_cache_for_trial_or_live_service( key_type=api_key.key_type, reference="ref2") - assert mock_incr.call_count == 2 + assert mock_incr.call_count == 1 mock_incr.assert_has_calls([ - call(str(service.id) + "-2016-01-01-count", ), + # call(str(service.id) + "-2016-01-01-count", ), call("2016-01-01-total", ) ]) @@ -247,9 +247,9 @@ def test_persist_notification_sets_daily_limit_cache_if_one_does_not_exists( key_type=api_key.key_type, reference="ref2") - assert mock_set.call_count == 2 + assert mock_set.call_count == 1 mock_set.assert_has_calls([ - call(str(service.id) + "-2016-01-01-count", 1, ex=86400), + # call(str(service.id) + "-2016-01-01-count", 1, ex=86400), call("2016-01-01-total", 1, ex=86400) ]) diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 52e797ebe..4f6e9767e 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -1,5 +1,3 @@ -from datetime import datetime - import pytest from flask import current_app from freezegun import freeze_time @@ -12,7 +10,7 @@ from app.notifications.process_notifications import ( create_content_for_notification, ) from app.notifications.validators import ( - check_application_over_daily_message_total, + check_application_over_retention_limit, check_if_service_can_send_files_by_email, check_is_message_too_long, check_notification_content_is_not_empty, @@ -20,7 +18,6 @@ from app.notifications.validators import ( check_reply_to, check_service_email_reply_to_id, check_service_over_api_rate_limit, - check_service_over_daily_message_limit, check_service_sms_sender_id, check_template_is_active, check_template_is_for_notification_type, @@ -34,12 +31,7 @@ from app.serialised_models import ( SerialisedTemplate, ) from app.utils import get_template_instance -from app.v2.errors import ( - BadRequestError, - RateLimitError, - TooManyRequestsError, - TotalRequestsError, -) +from app.v2.errors import BadRequestError, RateLimitError, TotalRequestsError from tests.app.db import ( create_api_key, create_reply_to_email, @@ -58,74 +50,13 @@ def enable_redis(notify_api): yield -@pytest.mark.parametrize('key_type', ['team', 'normal']) -def test_check_service_message_limit_in_cache_under_message_limit_passes( - key_type, - sample_service, - mocker): - serialised_service = SerialisedService.from_id(sample_service.id) - mock_get = mocker.patch('app.notifications.validators.redis_store.get', return_value="1") - mock_set = mocker.patch('app.notifications.validators.redis_store.set') - service_stats = check_service_over_daily_message_limit(key_type, serialised_service) - assert service_stats == 1 - mock_get.assert_called_once_with(f'{serialised_service.id}-{datetime.utcnow().strftime("%Y-%m-%d")}-count') - mock_set.assert_not_called() - - -def test_check_service_over_daily_message_limit_should_not_interact_with_cache_for_test_key(sample_service, mocker): - mocker.patch('app.notifications.validators.redis_store') - mock_get = mocker.patch('app.notifications.validators.redis_store.get', side_effect=[None]) - serialised_service = SerialisedService.from_id(sample_service.id) - service_stats = check_service_over_daily_message_limit('test', serialised_service) - assert service_stats == 0 - mock_get.assert_not_called() - - -@pytest.mark.parametrize('key_type', ['team', 'normal']) -def test_check_service_over_daily_message_limit_should_set_cache_value_as_zero_if_cache_not_set( - key_type, - sample_template, - sample_service, - mocker -): - serialised_service = SerialisedService.from_id(sample_service.id) - with freeze_time("2016-01-01 12:00:00.000000"): - mocker.patch('app.notifications.validators.redis_store.set') - service_stats = check_service_over_daily_message_limit(key_type, serialised_service) - app.notifications.validators.redis_store.set.assert_called_with( - str(sample_service.id) + "-2016-01-01-count", 0, ex=86400 - ) - assert service_stats == 0 - - -def test_check_service_over_daily_message_limit_does_nothing_if_redis_disabled(notify_api, sample_service, mocker): - serialised_service = SerialisedService.from_id(sample_service.id) - with set_config(notify_api, 'REDIS_ENABLED', False): - mock_cache_key = mocker.patch('notifications_utils.clients.redis.daily_limit_cache_key') - service_stats = check_service_over_daily_message_limit('normal', serialised_service) - assert service_stats == 0 - assert mock_cache_key.method_calls == [] - - -@pytest.mark.parametrize('key_type', ['team', 'normal']) -def test_check_service_message_limit_over_message_limit_fails(key_type, mocker, notify_db_session): - service = create_service(message_limit=4) - mocker.patch('app.redis_store.get', return_value="5") - - with pytest.raises(TooManyRequestsError) as e: - check_service_over_daily_message_limit(key_type, service) - assert e.value.status_code == 429 - assert e.value.message == 'Exceeded send limits (4) for today' - assert e.value.fields == [] - - @pytest.mark.parametrize('key_type', ['team', 'normal']) def test_check_service_message_limit_over_total_limit_fails(key_type, mocker, notify_db_session): service = create_service() mocker.patch('app.redis_store.get', return_value="5001") with pytest.raises(TotalRequestsError) as e: - check_application_over_daily_message_total(key_type, service) + check_application_over_retention_limit(key_type, service) assert e.value.status_code == 429 assert e.value.message == 'Exceeded total application limits (5000) for today' assert e.value.fields == [] @@ -479,14 +410,12 @@ 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') - mock_daily_limit = mocker.patch('app.notifications.validators.check_service_over_daily_message_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) - mock_daily_limit.assert_called_once_with(api_key.key_type, service) @pytest.mark.parametrize('key_type', ['test', 'normal']) diff --git a/tests/app/service/send_notification/test_send_one_off_notification.py b/tests/app/service/send_notification/test_send_one_off_notification.py index fcc458b02..159de94c0 100644 --- a/tests/app/service/send_notification/test_send_one_off_notification.py +++ b/tests/app/service/send_notification/test_send_one_off_notification.py @@ -19,7 +19,7 @@ from app.models import ( ServiceGuestList, ) from app.service.send_notification import send_one_off_notification -from app.v2.errors import BadRequestError, TooManyRequestsError +from app.v2.errors import BadRequestError from tests.app.db import ( create_reply_to_email, create_service, @@ -235,24 +235,6 @@ def test_send_one_off_notification_raises_if_cant_send_to_recipient( assert 'service is in trial mode' in e.value.message -def test_send_one_off_notification_raises_if_over_limit(notify_db_session, mocker): - service = create_service(message_limit=0) - template = create_template(service=service) - mocker.patch( - 'app.service.send_notification.check_service_over_daily_message_limit', - side_effect=TooManyRequestsError(1) - ) - - post_data = { - 'template_id': str(template.id), - 'to': '07700 900 001', - 'created_by': str(service.created_by_id) - } - - with pytest.raises(TooManyRequestsError): - send_one_off_notification(service.id, post_data) - - def test_send_one_off_notification_raises_if_message_too_long(persist_mock, notify_db_session): service = create_service() template = create_template(service=service, content="Hello (( Name))\nYour thing is due soon")