From 1b4097cb16ccfbe9485632d998a9f7854f1016fc Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 7 Jun 2017 11:15:05 +0100 Subject: [PATCH] Add three new columns to job_statistics for sent, delivered and failed. A job only ever has one notification type. This is the first deploy, where the columns are added and populated. Next a data migration will happen to populate these new columns for the older jobs that do not have the values set. Then we stop populating the old columns and remove them. This refactoring of the table structure will make the queries to the table much easier to handle. --- app/dao/statistics_dao.py | 17 +- app/models.py | 3 + migrations/versions/0094_job_stats_update.py | 25 +++ tests/app/dao/test_statistics_dao.py | 215 ++++++++++++++----- 4 files changed, 203 insertions(+), 57 deletions(-) create mode 100644 migrations/versions/0094_job_stats_update.py diff --git a/app/dao/statistics_dao.py b/app/dao/statistics_dao.py index baff82ba4..48d62e71c 100644 --- a/app/dao/statistics_dao.py +++ b/app/dao/statistics_dao.py @@ -60,7 +60,10 @@ def timeout_job_counts(notifications_type, timeout_start): ).update({ sent: sent_count, failed: failed_count, - delivered: delivered_count + delivered: delivered_count, + 'sent': sent_count, + 'delivered': delivered_count, + 'failed': failed_count }, synchronize_session=False) return total_updated @@ -87,11 +90,13 @@ def create_or_update_job_sending_statistics(notification): @transactional def __update_job_stats_sent_count(notification): column = columns(notification.notification_type, 'sent') + new_column = 'sent' return db.session.query(JobStatistics).filter_by( job_id=notification.job_id, ).update({ - column: column + 1 + column: column + 1, + new_column: column + 1 }) @@ -102,7 +107,8 @@ def __insert_job_stats(notification): emails_sent=1 if notification.notification_type == EMAIL_TYPE else 0, sms_sent=1 if notification.notification_type == SMS_TYPE else 0, letters_sent=1 if notification.notification_type == LETTER_TYPE else 0, - updated_at=datetime.utcnow() + updated_at=datetime.utcnow(), + sent=1 ) db.session.add(stats) @@ -131,10 +137,12 @@ def columns(notification_type, status): def update_job_stats_outcome_count(notification): if notification.status in NOTIFICATION_STATUS_TYPES_FAILED: column = columns(notification.notification_type, 'failed') + new_column = 'failed' elif notification.status in [NOTIFICATION_DELIVERED, NOTIFICATION_SENT] and notification.notification_type != LETTER_TYPE: column = columns(notification.notification_type, 'delivered') + new_column = 'delivered' else: column = None @@ -143,7 +151,8 @@ def update_job_stats_outcome_count(notification): return db.session.query(JobStatistics).filter_by( job_id=notification.job_id, ).update({ - column: column + 1 + column: column + 1, + new_column: column + 1 }) else: return 0 diff --git a/app/models.py b/app/models.py index 1bcd66a19..22d819389 100644 --- a/app/models.py +++ b/app/models.py @@ -1122,6 +1122,9 @@ class JobStatistics(db.Model): sms_failed = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) letters_sent = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) letters_failed = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) + sent = db.Column(db.BigInteger, index=False, unique=False, nullable=True, default=0) + delivered = db.Column(db.BigInteger, index=False, unique=False, nullable=True, default=0) + failed = db.Column(db.BigInteger, index=False, unique=False, nullable=True, default=0) created_at = db.Column( db.DateTime, index=False, diff --git a/migrations/versions/0094_job_stats_update.py b/migrations/versions/0094_job_stats_update.py new file mode 100644 index 000000000..6a7f7db2a --- /dev/null +++ b/migrations/versions/0094_job_stats_update.py @@ -0,0 +1,25 @@ +"""empty message + +Revision ID: 0094_job_stats_update +Revises: 0093_data_gov_uk +Create Date: 2017-06-06 14:37:30.051647 + +""" +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision = '0094_job_stats_update' +down_revision = '0093_data_gov_uk' + + +def upgrade(): + op.add_column('job_statistics', sa.Column('sent', sa.BigInteger(), nullable=True)) + op.add_column('job_statistics', sa.Column('delivered', sa.BigInteger(), nullable=True)) + op.add_column('job_statistics', sa.Column('failed', sa.BigInteger(), nullable=True)) + + +def downgrade(): + op.drop_column('job_statistics', 'sent') + op.drop_column('job_statistics', 'failed') + op.drop_column('job_statistics', 'delivered') diff --git a/tests/app/dao/test_statistics_dao.py b/tests/app/dao/test_statistics_dao.py index 4425a3409..5a73ea672 100644 --- a/tests/app/dao/test_statistics_dao.py +++ b/tests/app/dao/test_statistics_dao.py @@ -199,6 +199,10 @@ def test_should_update_a_stats_entry_with_its_success_outcome_for_a_job( assert stat.sms_failed == 0 assert stat.letters_failed == 0 + assert stat.sent == email_count + sms_count + letter_count + assert stat.delivered == email_count + sms_count + assert stat.failed == 0 + @pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count, status', [ (SMS_TYPE, 1, 0, 0, NOTIFICATION_TECHNICAL_FAILURE), @@ -264,6 +268,10 @@ def test_should_update_a_stats_entry_with_its_error_outcomes_for_a_job( assert stat.emails_delivered == 0 assert stat.sms_delivered == 0 + assert stat.sent == email_count + sms_count + letter_count + assert stat.delivered == 0 + assert stat.failed == email_count + sms_count + letter_count + @pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count, status', [ (SMS_TYPE, 1, 0, 0, NOTIFICATION_DELIVERED), @@ -326,6 +334,10 @@ def test_should_update_a_stats_entry_with_its_success_outcomes_for_a_job( assert stat.emails_delivered == email_count assert stat.sms_delivered == sms_count + assert stat.sent == email_count + sms_count + letter_count + assert stat.delivered == 0 if notification_type == LETTER_TYPE else 1 + assert stat.failed == 0 + @pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count, status', [ (SMS_TYPE, 1, 0, 0, NOTIFICATION_PENDING), @@ -394,6 +406,10 @@ def test_should_not_update_job_stats_if_irrelevant_status( assert stat.emails_delivered == 0 assert stat.sms_delivered == 0 + assert stat.sent == email_count + sms_count + letter_count + assert stat.delivered == 0 + assert stat.failed == 0 + @pytest.mark.parametrize('notification_type, sms_count, email_count, letter_count', [ (SMS_TYPE, 2, 1, 1), @@ -480,41 +496,52 @@ def test_inserting_one_type_of_notification_maintains_other_counts( assert updated_stats[0].sms_sent == sms_count assert updated_stats[0].letters_sent == letter_count + if notification_type == EMAIL_TYPE: + assert updated_stats[0].sent == email_count + elif notification_type == SMS_TYPE: + assert updated_stats[0].sent == sms_count + elif notification_type == LETTER_TYPE: + assert updated_stats[0].sent == letter_count + def test_updating_one_type_of_notification_to_success_maintains_other_counts( notify_db, notify_db_session, - sample_job, + sample_service, sample_letter_template ): - sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) - email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + job_1 = sample_job(notify_db, notify_db_session, service=sample_service) + job_2 = sample_job(notify_db, notify_db_session, service=sample_service) + job_3 = sample_job(notify_db, notify_db_session, service=sample_service) + + sms_template = sample_template(notify_db, notify_db_session, service=sample_service) + email_template = sample_email_template(notify_db, notify_db_session, service=sample_service) letter_template = sample_letter_template letter = sample_notification( notify_db, notify_db_session, - service=sample_job.service, + service=sample_service, template=letter_template, - job=sample_job, + job=job_1, status=NOTIFICATION_CREATED ) email = sample_notification( notify_db, notify_db_session, - service=sample_job.service, + service=sample_service, template=email_template, - job=sample_job, + job=job_2, status=NOTIFICATION_CREATED ) sms = sample_notification( notify_db, notify_db_session, - service=sample_job.service, + service=sample_service, template=sms_template, - job=sample_job, + job=job_3, status=NOTIFICATION_CREATED ) @@ -530,49 +557,76 @@ def test_updating_one_type_of_notification_to_success_maintains_other_counts( update_job_stats_outcome_count(email) update_job_stats_outcome_count(sms) - stats = JobStatistics.query.all() - assert len(stats) == 1 - assert stats[0].emails_sent == 1 - assert stats[0].sms_sent == 1 + stats = JobStatistics.query.order_by(JobStatistics.created_at).all() + assert len(stats) == 3 assert stats[0].letters_sent == 1 - assert stats[0].emails_delivered == 1 - assert stats[0].sms_delivered == 1 + assert stats[0].emails_sent == 0 + assert stats[0].sms_sent == 0 + assert stats[0].emails_delivered == 0 + assert stats[0].sms_delivered == 0 + + assert stats[1].letters_sent == 0 + assert stats[1].emails_sent == 1 + assert stats[1].sms_sent == 0 + assert stats[1].emails_delivered == 1 + assert stats[1].sms_delivered == 0 + + assert stats[2].letters_sent == 0 + assert stats[2].emails_sent == 0 + assert stats[2].sms_sent == 1 + assert stats[2].emails_delivered == 0 + assert stats[2].sms_delivered == 1 + + assert stats[0].sent == 1 + assert stats[0].delivered == 0 + assert stats[0].failed == 0 + + assert stats[1].sent == 1 + assert stats[1].delivered == 1 + assert stats[1].failed == 0 + + assert stats[2].sent == 1 + assert stats[2].delivered == 1 + assert stats[2].failed == 0 def test_updating_one_type_of_notification_to_error_maintains_other_counts( notify_db, notify_db_session, - sample_job, + sample_service, sample_letter_template ): - sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) - email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) + job_1 = sample_job(notify_db, notify_db_session, service=sample_service) + job_2 = sample_job(notify_db, notify_db_session, service=sample_service) + job_3 = sample_job(notify_db, notify_db_session, service=sample_service) + sms_template = sample_template(notify_db, notify_db_session, service=sample_service) + email_template = sample_email_template(notify_db, notify_db_session, service=sample_service) letter_template = sample_letter_template letter = sample_notification( notify_db, notify_db_session, - service=sample_job.service, + service=sample_service, template=letter_template, - job=sample_job, + job=job_1, status=NOTIFICATION_CREATED ) email = sample_notification( notify_db, notify_db_session, - service=sample_job.service, + service=sample_service, template=email_template, - job=sample_job, + job=job_2, status=NOTIFICATION_CREATED ) sms = sample_notification( notify_db, notify_db_session, - service=sample_job.service, + service=sample_service, template=sms_template, - job=sample_job, + job=job_3, status=NOTIFICATION_CREATED ) @@ -588,20 +642,50 @@ def test_updating_one_type_of_notification_to_error_maintains_other_counts( update_job_stats_outcome_count(email) update_job_stats_outcome_count(sms) - stats = JobStatistics.query.all() - assert len(stats) == 1 - assert stats[0].emails_sent == 1 - assert stats[0].sms_sent == 1 + stats = JobStatistics.query.order_by(JobStatistics.created_at).all() + assert len(stats) == 3 + assert stats[0].emails_sent == 0 + assert stats[0].sms_sent == 0 assert stats[0].letters_sent == 1 assert stats[0].emails_delivered == 0 assert stats[0].sms_delivered == 0 - assert stats[0].sms_failed == 1 - assert stats[0].emails_failed == 1 + assert stats[0].sms_failed == 0 + assert stats[0].emails_failed == 0 + assert stats[0].letters_failed == 1 + + assert stats[1].emails_sent == 1 + assert stats[1].sms_sent == 0 + assert stats[1].letters_sent == 0 + assert stats[1].emails_delivered == 0 + assert stats[1].sms_delivered == 0 + assert stats[1].sms_failed == 0 + assert stats[1].emails_failed == 1 + assert stats[1].letters_failed == 0 + + assert stats[2].emails_sent == 0 + assert stats[2].sms_sent == 1 + assert stats[2].letters_sent == 0 + assert stats[2].emails_delivered == 0 + assert stats[2].sms_delivered == 0 + assert stats[2].sms_failed == 1 + assert stats[2].emails_failed == 0 + assert stats[1].letters_failed == 0 + + assert stats[0].sent == 1 + assert stats[0].delivered == 0 + assert stats[0].failed == 1 + + assert stats[1].sent == 1 + assert stats[1].delivered == 0 + assert stats[1].failed == 1 + + assert stats[2].sent == 1 + assert stats[2].delivered == 0 + assert stats[2].failed == 1 -def test_will_not_timeout_job_counts_before_notification_timeouts(notify_db, notify_db_session, sample_job): - sms_template = sample_template(notify_db, notify_db_session, service=sample_job.service) - email_template = sample_email_template(notify_db, notify_db_session, service=sample_job.service) +def test_will_not_timeout_job_counts_before_notification_timeouts(notify_db, notify_db_session, + sample_job, sample_template): one_minute_ago = datetime.utcnow() - timedelta(minutes=1) @@ -609,43 +693,51 @@ def test_will_not_timeout_job_counts_before_notification_timeouts(notify_db, not notify_db, notify_db_session, service=sample_job.service, - template=sms_template, + template=sample_template, job=sample_job, status=NOTIFICATION_CREATED ) - email = sample_notification( + sms_2 = sample_notification( notify_db, notify_db_session, service=sample_job.service, - template=email_template, + template=sample_template, job=sample_job, status=NOTIFICATION_CREATED ) - create_or_update_job_sending_statistics(email) create_or_update_job_sending_statistics(sms) + create_or_update_job_sending_statistics(sms_2) JobStatistics.query.update({JobStatistics.created_at: one_minute_ago}) - intial_stats = JobStatistics.query.all() + initial_stats = JobStatistics.query.all() - assert intial_stats[0].emails_sent == 1 - assert intial_stats[0].sms_sent == 1 - assert intial_stats[0].emails_delivered == 0 - assert intial_stats[0].sms_delivered == 0 - assert intial_stats[0].sms_failed == 0 - assert intial_stats[0].emails_failed == 0 + assert initial_stats[0].emails_sent == 0 + assert initial_stats[0].sms_sent == 2 + assert initial_stats[0].emails_delivered == 0 + assert initial_stats[0].sms_delivered == 0 + assert initial_stats[0].sms_failed == 0 + assert initial_stats[0].emails_failed == 0 + + assert initial_stats[0].sent == 2 + assert initial_stats[0].delivered == 0 + assert initial_stats[0].failed == 0 dao_timeout_job_statistics(61) updated_stats = JobStatistics.query.all() - assert updated_stats[0].emails_sent == 1 - assert updated_stats[0].sms_sent == 1 + assert updated_stats[0].emails_sent == 0 + assert updated_stats[0].sms_sent == 2 assert updated_stats[0].emails_delivered == 0 assert updated_stats[0].sms_delivered == 0 assert updated_stats[0].sms_failed == 0 assert updated_stats[0].emails_failed == 0 + assert initial_stats[0].sent == 2 + assert initial_stats[0].delivered == 0 + assert initial_stats[0].failed == 0 + @pytest.mark.parametrize('notification_type, sms_count, email_count', [ (SMS_TYPE, 3, 0), @@ -688,6 +780,9 @@ def test_timeout_job_counts_timesout_multiple_jobs( assert stats.sms_delivered == 0 assert stats.sms_failed == 0 assert stats.emails_failed == 0 + assert stats.sent == email_count + sms_count + assert stats.delivered == 0 + assert stats.failed == 0 dao_timeout_job_statistics(1) updated_stats = JobStatistics.query.all() @@ -698,6 +793,9 @@ def test_timeout_job_counts_timesout_multiple_jobs( assert stats.sms_delivered == 0 assert stats.sms_failed == sms_count assert stats.emails_failed == email_count + assert stats.sent == email_count + sms_count + assert stats.delivered == 0 + assert stats.failed == email_count + sms_count count_notifications = len(NOTIFICATION_STATUS_TYPES) @@ -754,17 +852,13 @@ def test_timeout_job_sets_all_non_delivered_emails_to_error_and_doesnt_affect_sm assert initial_stats[0].sms_failed == 0 assert initial_stats[0].emails_failed == 0 - all = JobStatistics.query.all() - for a in all: - print(a) + assert initial_stats[0].sent == count_notifications + assert initial_stats[0].delivered == 0 + assert initial_stats[0].failed == 0 # timeout the notifications dao_timeout_job_statistics(1) - all = JobStatistics.query.all() - for a in all: - print(a) - # after timeout all delivered states are success and ALL other states are failed updated_stats = JobStatistics.query.filter_by(job_id=email_job.id).all() assert updated_stats[0].emails_sent == count_notifications @@ -774,6 +868,10 @@ def test_timeout_job_sets_all_non_delivered_emails_to_error_and_doesnt_affect_sm assert updated_stats[0].sms_failed == 0 assert updated_stats[0].emails_failed == count_error_notifications + assert initial_stats[0].sent == count_notifications + assert initial_stats[0].delivered == count_success_notifications + assert initial_stats[0].failed == count_error_notifications + sms_stats = JobStatistics.query.filter_by(job_id=sms_job.id).all() assert sms_stats[0].emails_sent == 0 assert sms_stats[0].sms_sent == 1 @@ -781,6 +879,9 @@ def test_timeout_job_sets_all_non_delivered_emails_to_error_and_doesnt_affect_sm assert sms_stats[0].sms_delivered == 0 assert sms_stats[0].sms_failed == 1 assert sms_stats[0].emails_failed == 0 + assert sms_stats[0].sent == 1 + assert sms_stats[0].delivered == 0 + assert sms_stats[0].failed == 1 # this test is as above, but for SMS not email @@ -810,6 +911,10 @@ def test_timeout_job_sets_all_non_delivered_states_to_error( assert stats.sms_failed == 0 assert stats.emails_failed == 0 + assert stats.sent == count_notifications + assert stats.delivered == 0 + assert stats.failed == 0 + dao_timeout_job_statistics(1) updated_stats = JobStatistics.query.all() @@ -820,3 +925,7 @@ def test_timeout_job_sets_all_non_delivered_states_to_error( assert stats.sms_delivered == count_success_notifications assert stats.sms_failed == count_error_notifications assert stats.emails_failed == 0 + + assert stats.sent == count_notifications + assert stats.delivered == count_success_notifications + assert stats.failed == count_error_notifications