From 17612e544631463d4cf80b04f3548c400b04b989 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 24 Sep 2018 15:36:05 +0100 Subject: [PATCH 1/3] add postage constraint to notification history A not valid constraint only checks against new rows, not existing rows. We can call VALIDATE CONSTRAINT against this new constraint to check the old rows (which we know are good, having run the command from 74961781). Adding a normal constraint acquires an ACCESS EXCLUSIVE lock, but validate constraint only needs a SHARE UPDATE EXCLUSIVE lock. see 9d4b8961 and 0a50993f for more information on marking constraints as not valid. --- app/models.py | 16 ++++++- .../versions/0230_noti_postage_constraint.py | 44 +++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 migrations/versions/0230_noti_postage_constraint.py diff --git a/app/models.py b/app/models.py index fbd1b7419..cf58a6867 100644 --- a/app/models.py +++ b/app/models.py @@ -1191,7 +1191,13 @@ class Notification(db.Model): reply_to_text = db.Column(db.String, nullable=True) postage = db.Column(db.String, nullable=True) - CheckConstraint("notification_type != 'letter' or postage in ('first', 'second')") + CheckConstraint(""" + CASE WHEN notification_type = 'letter' THEN + postage in ('first', 'second') + ELSE + postage is null + END + """) __table_args__ = ( db.ForeignKeyConstraint( @@ -1445,7 +1451,13 @@ class NotificationHistory(db.Model, HistoryModel): created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), nullable=True) postage = db.Column(db.String, nullable=True) - CheckConstraint("notification_type != 'letter' or postage in ('first', 'second')") + CheckConstraint(""" + CASE WHEN notification_type = 'letter' THEN + postage in ('first', 'second') + ELSE + postage is null + END + """) __table_args__ = ( db.ForeignKeyConstraint( diff --git a/migrations/versions/0230_noti_postage_constraint.py b/migrations/versions/0230_noti_postage_constraint.py new file mode 100644 index 000000000..118d51b39 --- /dev/null +++ b/migrations/versions/0230_noti_postage_constraint.py @@ -0,0 +1,44 @@ +""" + +Revision ID: 0230_noti_postage_constraint +Revises: 0229_new_letter_rates +Create Date: 2018-09-19 11:42:52.229430 + +""" +from alembic import op + + +revision = '0230_noti_postage_constraint' +down_revision = '0229_new_letter_rates' + + +def upgrade(): + op.execute(""" + ALTER TABLE notifications ADD CONSTRAINT "chk_notifications_postage_null" + CHECK ( + CASE WHEN notification_type = 'letter' THEN + postage in ('first', 'second') + ELSE + postage is null + END + ) + NOT VALID + """) + op.execute(""" + ALTER TABLE notification_history ADD CONSTRAINT "chk_notification_history_postage_null" + CHECK ( + CASE WHEN notification_type = 'letter' THEN + postage in ('first', 'second') + ELSE + postage is null + END + ) + NOT VALID + """) + op.execute('ALTER TABLE notifications VALIDATE CONSTRAINT "chk_notifications_postage_null"') + op.execute('ALTER TABLE notification_history VALIDATE CONSTRAINT "chk_notification_history_postage_null"') + + +def downgrade(): + op.drop_constraint('chk_notifications_postage_null', 'notifications', type_='check') + op.drop_constraint('chk_notification_history_postage_null', 'notification_history', type_='check') From 3739c573eab6f73b88ff2d96198b558e96ccf0c8 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 25 Sep 2018 14:06:36 +0100 Subject: [PATCH 2/3] fix null check in constraint There are two fun quirks of postgres/sql that we need to work around: * any `x = y` where x or y is NULL returns NULL, rather than false. * check constraints accept NULL or true values as good. so, the check `postage in ('first', 'second')` returns `null` rather than `false` when postage is null itself. This surprisingly passes the check constraint. To get around this, we have to add an explicit not null check as well. --- app/models.py | 4 ++-- migrations/versions/0230_noti_postage_constraint.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models.py b/app/models.py index cf58a6867..5df7730bf 100644 --- a/app/models.py +++ b/app/models.py @@ -1193,7 +1193,7 @@ class Notification(db.Model): postage = db.Column(db.String, nullable=True) CheckConstraint(""" CASE WHEN notification_type = 'letter' THEN - postage in ('first', 'second') + postage is not null and postage in ('first', 'second') ELSE postage is null END @@ -1453,7 +1453,7 @@ class NotificationHistory(db.Model, HistoryModel): postage = db.Column(db.String, nullable=True) CheckConstraint(""" CASE WHEN notification_type = 'letter' THEN - postage in ('first', 'second') + postage is not null and postage in ('first', 'second') ELSE postage is null END diff --git a/migrations/versions/0230_noti_postage_constraint.py b/migrations/versions/0230_noti_postage_constraint.py index 118d51b39..a8c40d7ec 100644 --- a/migrations/versions/0230_noti_postage_constraint.py +++ b/migrations/versions/0230_noti_postage_constraint.py @@ -17,7 +17,7 @@ def upgrade(): ALTER TABLE notifications ADD CONSTRAINT "chk_notifications_postage_null" CHECK ( CASE WHEN notification_type = 'letter' THEN - postage in ('first', 'second') + postage is not null and postage in ('first', 'second') ELSE postage is null END @@ -28,7 +28,7 @@ def upgrade(): ALTER TABLE notification_history ADD CONSTRAINT "chk_notification_history_postage_null" CHECK ( CASE WHEN notification_type = 'letter' THEN - postage in ('first', 'second') + postage is not null and postage in ('first', 'second') ELSE postage is null END From 69e08fa23b0776e93aef1bf331c6b9092febcb13 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 25 Sep 2018 15:59:08 +0100 Subject: [PATCH 3/3] fix tests to create notifications properly migrated away from calling fixtures as functions --- tests/app/celery/test_reporting_tasks.py | 94 ++++--------------- .../test_total_sent_notifications.py | 37 +++----- tests/app/test_model.py | 48 +++------- 3 files changed, 46 insertions(+), 133 deletions(-) diff --git a/tests/app/celery/test_reporting_tasks.py b/tests/app/celery/test_reporting_tasks.py index f21de04c2..8da479555 100644 --- a/tests/app/celery/test_reporting_tasks.py +++ b/tests/app/celery/test_reporting_tasks.py @@ -1,5 +1,10 @@ from datetime import datetime, timedelta, date -from tests.app.conftest import sample_notification +from decimal import Decimal + +import pytest +from freezegun import freeze_time +from sqlalchemy import desc + from app.celery.reporting_tasks import create_nightly_billing, create_nightly_notification_status from app.dao.fact_billing_dao import get_rate from app.models import ( @@ -9,12 +14,8 @@ from app.models import ( EMAIL_TYPE, SMS_TYPE, FactNotificationStatus ) -from decimal import Decimal -import pytest from app.models import LetterRate, Rate from app import db -from freezegun import freeze_time -from sqlalchemy import desc from tests.app.db import create_service, create_template, create_notification @@ -38,8 +39,6 @@ def mocker_get_rate( [(1.0, 1, 2, [1]), (2.0, 2, 1, [1, 2])]) def test_create_nightly_billing_sms_rate_multiplier( - notify_db, - notify_db_session, sample_service, sample_template, mocker, @@ -53,11 +52,8 @@ def test_create_nightly_billing_sms_rate_multiplier( mocker.patch('app.dao.fact_billing_dao.get_rate', side_effect=mocker_get_rate) # These are sms notifications - sample_notification( - notify_db, - notify_db_session, + create_notification( created_at=yesterday, - service=sample_service, template=sample_template, status='delivered', sent_by='mmg', @@ -65,11 +61,8 @@ def test_create_nightly_billing_sms_rate_multiplier( rate_multiplier=1.0, billable_units=1, ) - sample_notification( - notify_db, - notify_db_session, + create_notification( created_at=yesterday, - service=sample_service, template=sample_template, status='delivered', sent_by='mmg', @@ -94,8 +87,6 @@ def test_create_nightly_billing_sms_rate_multiplier( def test_create_nightly_billing_different_templates( - notify_db, - notify_db_session, sample_service, sample_template, sample_email_template, @@ -104,11 +95,8 @@ def test_create_nightly_billing_different_templates( mocker.patch('app.dao.fact_billing_dao.get_rate', side_effect=mocker_get_rate) - sample_notification( - notify_db, - notify_db_session, + create_notification( created_at=yesterday, - service=sample_service, template=sample_template, status='delivered', sent_by='mmg', @@ -116,11 +104,8 @@ def test_create_nightly_billing_different_templates( rate_multiplier=1.0, billable_units=1, ) - sample_notification( - notify_db, - notify_db_session, + create_notification( created_at=yesterday, - service=sample_service, template=sample_email_template, status='delivered', sent_by='ses', @@ -148,8 +133,6 @@ def test_create_nightly_billing_different_templates( def test_create_nightly_billing_different_sent_by( - notify_db, - notify_db_session, sample_service, sample_template, sample_email_template, @@ -159,11 +142,8 @@ def test_create_nightly_billing_different_sent_by( mocker.patch('app.dao.fact_billing_dao.get_rate', side_effect=mocker_get_rate) # These are sms notifications - sample_notification( - notify_db, - notify_db_session, + create_notification( created_at=yesterday, - service=sample_service, template=sample_template, status='delivered', sent_by='mmg', @@ -171,11 +151,8 @@ def test_create_nightly_billing_different_sent_by( rate_multiplier=1.0, billable_units=1, ) - sample_notification( - notify_db, - notify_db_session, + create_notification( created_at=yesterday, - service=sample_service, template=sample_template, status='delivered', sent_by='firetext', @@ -201,8 +178,6 @@ def test_create_nightly_billing_different_sent_by( def test_create_nightly_billing_letter( - notify_db, - notify_db_session, sample_service, sample_letter_template, mocker): @@ -210,11 +185,8 @@ def test_create_nightly_billing_letter( mocker.patch('app.dao.fact_billing_dao.get_rate', side_effect=mocker_get_rate) - sample_notification( - notify_db, - notify_db_session, + create_notification( created_at=yesterday, - service=sample_service, template=sample_letter_template, status='delivered', sent_by='dvla', @@ -239,8 +211,6 @@ def test_create_nightly_billing_letter( def test_create_nightly_billing_null_sent_by_sms( - notify_db, - notify_db_session, sample_service, sample_template, mocker): @@ -248,11 +218,8 @@ def test_create_nightly_billing_null_sent_by_sms( mocker.patch('app.dao.fact_billing_dao.get_rate', side_effect=mocker_get_rate) - sample_notification( - notify_db, - notify_db_session, + create_notification( created_at=yesterday, - service=sample_service, template=sample_template, status='delivered', sent_by=None, @@ -280,8 +247,6 @@ def test_create_nightly_billing_null_sent_by_sms( @freeze_time('2018-01-15T03:30:00') def test_create_nightly_billing_consolidate_from_3_days_delta( - notify_db, - notify_db_session, sample_service, sample_template, mocker): @@ -290,11 +255,8 @@ def test_create_nightly_billing_consolidate_from_3_days_delta( # create records from 11th to 15th for i in range(0, 11): - sample_notification( - notify_db, - notify_db_session, + create_notification( created_at=datetime.now() - timedelta(days=i), - service=sample_service, template=sample_template, status='delivered', sent_by=None, @@ -331,7 +293,7 @@ def test_get_rate_for_letter_latest(notify_db_session): assert rate == Decimal(0.33) -def test_get_rate_for_sms_and_email(notify_db, notify_db_session): +def test_get_rate_for_sms_and_email(notify_db_session): sms_rate = Rate(valid_from=datetime(2017, 12, 1), rate=Decimal(0.15), notification_type=SMS_TYPE) @@ -356,19 +318,14 @@ def test_get_rate_for_sms_and_email(notify_db, notify_db_session): @freeze_time('2018-03-27T03:30:00') # summer time starts on 2018-03-25 def test_create_nightly_billing_use_BST( - notify_db, - notify_db_session, sample_service, sample_template, mocker): mocker.patch('app.dao.fact_billing_dao.get_rate', side_effect=mocker_get_rate) - sample_notification( - notify_db, - notify_db_session, + create_notification( created_at=datetime(2018, 3, 25, 12, 0), - service=sample_service, template=sample_template, status='delivered', sent_by=None, @@ -377,11 +334,8 @@ def test_create_nightly_billing_use_BST( billable_units=1, ) - sample_notification( - notify_db, - notify_db_session, + create_notification( created_at=datetime(2018, 3, 25, 23, 5), - service=sample_service, template=sample_template, status='delivered', sent_by=None, @@ -405,19 +359,14 @@ def test_create_nightly_billing_use_BST( @freeze_time('2018-01-15T03:30:00') def test_create_nightly_billing_update_when_record_exists( - notify_db, - notify_db_session, sample_service, sample_template, mocker): mocker.patch('app.dao.fact_billing_dao.get_rate', side_effect=mocker_get_rate) - sample_notification( - notify_db, - notify_db_session, + create_notification( created_at=datetime.now() - timedelta(days=1), - service=sample_service, template=sample_template, status='delivered', sent_by=None, @@ -437,11 +386,8 @@ def test_create_nightly_billing_update_when_record_exists( assert records[0].billable_units == 1 assert not records[0].updated_at - sample_notification( - notify_db, - notify_db_session, + create_notification( created_at=datetime.now() - timedelta(days=1), - service=sample_service, template=sample_template, status='delivered', sent_by=None, diff --git a/tests/app/performance_platform/test_total_sent_notifications.py b/tests/app/performance_platform/test_total_sent_notifications.py index 3aecf7447..e00631abd 100644 --- a/tests/app/performance_platform/test_total_sent_notifications.py +++ b/tests/app/performance_platform/test_total_sent_notifications.py @@ -1,5 +1,4 @@ from datetime import datetime -from functools import partial from freezegun import freeze_time @@ -9,9 +8,7 @@ from app.performance_platform.total_sent_notifications import ( get_total_sent_notifications_for_day ) -from tests.app.conftest import ( - sample_notification_history as create_notification_history -) +from tests.app.db import create_template, create_notification def test_send_total_notifications_sent_for_day_stats_stats_creates_correct_call(mocker, client): @@ -37,32 +34,24 @@ def test_send_total_notifications_sent_for_day_stats_stats_creates_correct_call( @freeze_time("2016-01-11 12:30:00") -def test_get_total_sent_notifications_yesterday_returns_expected_totals_dict( - notify_db, - notify_db_session, - sample_template -): - notification_history = partial( - create_notification_history, - notify_db, - notify_db_session, - sample_template, - status='delivered' - ) +def test_get_total_sent_notifications_yesterday_returns_expected_totals_dict(sample_service): + sms = create_template(sample_service, template_type='sms') + email = create_template(sample_service, template_type='email') + letter = create_template(sample_service, template_type='letter') - notification_history(notification_type='email') - notification_history(notification_type='sms') + create_notification(email, status='delivered') + create_notification(sms, status='delivered') # Create some notifications for the day before yesterday = datetime(2016, 1, 10, 15, 30, 0, 0) ereyesterday = datetime(2016, 1, 9, 15, 30, 0, 0) with freeze_time(yesterday): - notification_history(notification_type='letter') - notification_history(notification_type='sms') - notification_history(notification_type='sms') - notification_history(notification_type='email') - notification_history(notification_type='email') - notification_history(notification_type='email') + create_notification(letter, status='delivered') + create_notification(sms, status='delivered') + create_notification(sms, status='delivered') + create_notification(email, status='delivered') + create_notification(email, status='delivered') + create_notification(email, status='delivered') total_count_dict = get_total_sent_notifications_for_day(yesterday) diff --git a/tests/app/test_model.py b/tests/app/test_model.py index d1d566ce1..74cf817ac 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -21,16 +21,14 @@ from app.models import ( NOTIFICATION_TECHNICAL_FAILURE, PRECOMPILED_TEMPLATE_NAME ) -from tests.app.conftest import ( - sample_template as create_sample_template, - sample_notification_with_job as create_sample_notification_with_job -) + from tests.app.db import ( create_notification, create_service, create_inbound_number, create_reply_to_email, - create_letter_contact + create_letter_contact, + create_template ) @@ -99,26 +97,17 @@ def test_status_conversion(initial_statuses, expected_statuses): ('sms', '+447700900855'), ('email', 'foo@bar.com'), ]) -def test_notification_for_csv_returns_correct_type(notify_db, notify_db_session, template_type, recipient): - template = create_sample_template(notify_db, notify_db_session, template_type=template_type) - notification = create_sample_notification_with_job( - notify_db, - notify_db_session, - template=template, - to_field=recipient - ) +def test_notification_for_csv_returns_correct_type(sample_service, template_type, recipient): + template = create_template(sample_service, template_type=template_type) + notification = create_notification(template, to_field=recipient) serialized = notification.serialize_for_csv() assert serialized['template_type'] == template_type @freeze_time("2016-01-01 11:09:00.000000") -def test_notification_for_csv_returns_correct_job_row_number(notify_db, notify_db_session): - notification = create_sample_notification_with_job( - notify_db, - notify_db_session, - job_row_number=0 - ) +def test_notification_for_csv_returns_correct_job_row_number(sample_job): + notification = create_notification(sample_job.template, sample_job, job_row_number=0) serialized = notification.serialize_for_csv() assert serialized['row_number'] == 1 @@ -139,32 +128,21 @@ def test_notification_for_csv_returns_correct_job_row_number(notify_db, notify_d ('letter', 'delivered', 'Received') ]) def test_notification_for_csv_returns_formatted_status( - notify_db, - notify_db_session, + sample_service, template_type, status, expected_status ): - template = create_sample_template(notify_db, notify_db_session, template_type=template_type) - notification = create_sample_notification_with_job( - notify_db, - notify_db_session, - status=status, - template=template - ) + template = create_template(sample_service, template_type=template_type) + notification = create_notification(template, status=status) serialized = notification.serialize_for_csv() assert serialized['status'] == expected_status @freeze_time("2017-03-26 23:01:53.321312") -def test_notification_for_csv_returns_bst_correctly(notify_db, notify_db_session): - notification = create_sample_notification_with_job( - notify_db, - notify_db_session, - job_row_number=100, - status='permanent-failure' - ) +def test_notification_for_csv_returns_bst_correctly(sample_template): + notification = create_notification(sample_template) serialized = notification.serialize_for_csv() assert serialized['created_at'] == 'Monday 27 March 2017 at 00:01'