From 88c878c83e2d514d08dd5bd16e14ba514d4da434 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 4 Dec 2017 11:12:26 +0000 Subject: [PATCH 1/2] don't hit the query to get daily msg stats if redis is disabled --- app/notifications/validators.py | 4 ++-- tests/app/notifications/test_validators.py | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 2fc01b64f..176241f1e 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -23,7 +23,7 @@ from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id def check_service_over_api_rate_limit(service, api_key): - if current_app.config['API_RATE_LIMIT_ENABLED']: + 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 = current_app.config['API_KEY_LIMITS'][api_key.key_type]['limit'] interval = current_app.config['API_KEY_LIMITS'][api_key.key_type]['interval'] @@ -33,7 +33,7 @@ def check_service_over_api_rate_limit(service, api_key): def check_service_over_daily_message_limit(key_type, service): - if key_type != KEY_TYPE_TEST: + if key_type != KEY_TYPE_TEST and current_app.config['REDIS_ENABLED']: cache_key = daily_limit_cache_key(service.id) service_stats = redis_store.get(cache_key) if not service_stats: diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index cb841e431..1c9fbb579 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -1,6 +1,7 @@ import pytest from freezegun import freeze_time from flask import current_app + import app from app.models import INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE from app.notifications.validators import ( @@ -18,6 +19,8 @@ from app.v2.errors import ( BadRequestError, TooManyRequestsError, RateLimitError) + +from tests.conftest import set_config from tests.app.conftest import ( sample_notification as create_notification, sample_service as create_service, @@ -26,6 +29,13 @@ from tests.app.conftest import ( from tests.app.db import create_reply_to_email, create_service_sms_sender +# all of these tests should have redis enabled (except where we specifically disable it) +@pytest.fixture(scope='module', autouse=True) +def enable_redis(notify_api): + with set_config(notify_api, 'REDIS_ENABLED', True): + yield + + @pytest.mark.parametrize('key_type', ['test', 'team', 'normal']) def test_check_service_message_limit_in_cache_with_unrestricted_service_is_allowed( key_type, @@ -78,6 +88,15 @@ def test_should_set_cache_value_as_value_from_database_if_cache_not_set( ) +def test_should_not_access_database_if_redis_disabled(notify_api, sample_service, mocker): + with set_config(notify_api, 'REDIS_ENABLED', False): + db_mock = mocker.patch('app.notifications.validators.services_dao') + + check_service_over_daily_message_limit('normal', sample_service) + + assert db_mock.method_calls == [] + + @pytest.mark.parametrize('key_type', ['team', 'normal']) def test_check_service_message_limit_over_message_limit_fails(key_type, notify_db, notify_db_session, mocker): with freeze_time("2016-01-01 12:00:00.000000"): From 5bb6b68e57181bb75d54f5e761cfbb92e04bb1a3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 4 Dec 2017 14:13:43 +0000 Subject: [PATCH 2/2] fix tests that expect to be rate limited (which is off if redis is disabled) --- .../notifications/rest/test_send_notification.py | 16 ++++++++++++---- tests/app/service/test_rest.py | 9 ++++----- .../service/test_send_one_off_notification.py | 6 +++++- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 4b57c4128..480455e75 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -16,7 +16,10 @@ from app.models import ( from app.dao.templates_dao import dao_get_all_templates_for_service, dao_update_template from app.dao.services_dao import dao_update_service from app.dao.api_key_dao import save_model_api_key -from app.v2.errors import RateLimitError +from app.errors import InvalidRequest +from app.models import Template +from app.v2.errors import RateLimitError, TooManyRequestsError + from tests import create_authorization_header from tests.app.conftest import ( sample_notification as create_sample_notification, @@ -28,9 +31,6 @@ from tests.app.conftest import ( sample_service, sample_template_without_sms_permission, sample_template_without_email_permission) - -from app.models import Template -from app.errors import InvalidRequest from tests.app.db import create_service, create_reply_to_email @@ -414,6 +414,10 @@ def test_should_block_api_call_if_over_day_limit_for_live_service( mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: + mocker.patch( + 'app.notifications.validators.check_service_over_daily_message_limit', + side_effect=TooManyRequestsError(1) + ) mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') service = create_sample_service(notify_db, notify_db_session, limit=1, restricted=False) @@ -446,6 +450,10 @@ def test_should_block_api_call_if_over_day_limit_for_restricted_service( with notify_api.test_request_context(): with notify_api.test_client() as client: mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch( + 'app.notifications.validators.check_service_over_daily_message_limit', + side_effect=TooManyRequestsError(1) + ) service = create_sample_service(notify_db, notify_db_session, limit=1, restricted=True) email_template = create_sample_email_template(notify_db, notify_db_session, service=service) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 73679001f..2c09939b0 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2163,18 +2163,17 @@ def test_search_for_notification_by_to_field_returns_content( assert notifications[0]['template']['content'] == 'Hello (( Name))\nYour thing is due soon' -def test_send_one_off_notification(admin_request, mocker): - service = create_service() - template = create_template(service=service) +def test_send_one_off_notification(sample_service, admin_request, mocker): + template = create_template(service=sample_service) mocker.patch('app.service.send_notification.send_notification_to_queue') response = admin_request.post( 'service.create_one_off_notification', - service_id=service.id, + service_id=sample_service.id, _data={ 'template_id': str(template.id), 'to': '07700900001', - 'created_by': str(service.created_by_id) + 'created_by': str(sample_service.created_by_id) }, _expected_status=201 ) diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index 14c49aa56..6a9a681ff 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -151,9 +151,13 @@ def test_send_one_off_notification_raises_if_cant_send_to_recipient(notify_db_se assert 'service is in trial mode' in e.value.message -def test_send_one_off_notification_raises_if_over_limit(notify_db_session): +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),