From 88c878c83e2d514d08dd5bd16e14ba514d4da434 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 4 Dec 2017 11:12:26 +0000 Subject: [PATCH 1/4] 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/4] 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), From c2228bb7d2aa6ec0f965a7d79a3b2b2b14134c6a Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 28 Nov 2017 11:59:17 +0000 Subject: [PATCH 3/4] Enable proxy header check on live --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index 22d9fdd10..1ba975619 100644 --- a/app/config.py +++ b/app/config.py @@ -417,7 +417,7 @@ class Live(Config): FUNCTIONAL_TEST_PROVIDER_SMS_TEMPLATE_ID = 'ba9e1789-a804-40b8-871f-cc60d4c1286f' PERFORMANCE_PLATFORM_ENABLED = True API_RATE_LIMIT_ENABLED = True - CHECK_PROXY_HEADER = False + CHECK_PROXY_HEADER = True class CloudFoundryConfig(Config): From b4a1d635f75029022d949c88f896ddf7afd1e1c8 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 5 Dec 2017 10:27:29 +0000 Subject: [PATCH 4/4] Rebuild the letter_rates table to include everything it needs. The current letter_rates table is not used so it is ok to drop and re-create it --- app/models.py | 15 ++---- .../versions/0150_refactor_letter_rates.py | 47 +++++++++++++++++++ 2 files changed, 52 insertions(+), 10 deletions(-) create mode 100644 migrations/versions/0150_refactor_letter_rates.py diff --git a/app/models.py b/app/models.py index 9d81073c7..0a65ae0a7 100644 --- a/app/models.py +++ b/app/models.py @@ -1484,17 +1484,12 @@ class LetterRate(db.Model): __tablename__ = 'letter_rates' id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - valid_from = valid_from = db.Column(db.DateTime, nullable=False) - - -class LetterRateDetail(db.Model): - __tablename__ = 'letter_rate_details' - - id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - letter_rate_id = db.Column(UUID(as_uuid=True), db.ForeignKey('letter_rates.id'), index=True, nullable=False) - letter_rate = db.relationship('LetterRate', backref='letter_rates') - page_total = db.Column(db.Integer, nullable=False) + start_date = db.Column(db.DateTime, nullable=False) + end_date = db.Column(db.DateTime, nullable=True) + sheet_total = db.Column(db.Integer, nullable=False) # double sided sheet rate = db.Column(db.Numeric(), nullable=False) + crown = db.Column(db.Boolean, nullable=False) + post_class = db.Column(db.String, nullable=False) class MonthlyBilling(db.Model): diff --git a/migrations/versions/0150_refactor_letter_rates.py b/migrations/versions/0150_refactor_letter_rates.py new file mode 100644 index 000000000..2429e5222 --- /dev/null +++ b/migrations/versions/0150_refactor_letter_rates.py @@ -0,0 +1,47 @@ +""" + +Revision ID: 0150_refactor_letter_rates +Revises: 0148_add_letters_as_pdf_svc_perm +Create Date: 2017-12-05 10:24:41.232128 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0150_refactor_letter_rates' +down_revision = '0148_add_letters_as_pdf_svc_perm' + + +def upgrade(): + op.drop_table('letter_rate_details') + op.drop_table('letter_rates') + op.create_table('letter_rates', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('start_date', sa.DateTime(), nullable=False), + sa.Column('end_date', sa.DateTime(), nullable=True), + sa.Column('sheet_total', sa.Integer(), nullable=False), + sa.Column('rate', sa.Numeric(), nullable=False), + sa.Column('crown', sa.Boolean(), nullable=False), + sa.Column('post_class', sa.String(), nullable=False), + sa.PrimaryKeyConstraint('id') + ) + + +def downgrade(): + op.drop_table('letter_rates') + op.create_table('letter_rates', + sa.Column('id', postgresql.UUID(), autoincrement=False, nullable=False), + sa.Column('valid_from', postgresql.TIMESTAMP(), autoincrement=False, nullable=False), + sa.PrimaryKeyConstraint('id', name='letter_rates_pkey'), + postgresql_ignore_search_path=False + ) + op.create_table('letter_rate_details', + sa.Column('id', postgresql.UUID(), autoincrement=False, nullable=False), + sa.Column('letter_rate_id', postgresql.UUID(), autoincrement=False, nullable=False), + sa.Column('page_total', sa.INTEGER(), autoincrement=False, nullable=False), + sa.Column('rate', sa.NUMERIC(), autoincrement=False, nullable=False), + sa.ForeignKeyConstraint(['letter_rate_id'], ['letter_rates.id'], + name='letter_rate_details_letter_rate_id_fkey'), + sa.PrimaryKeyConstraint('id', name='letter_rate_details_pkey') + )