From de9d1a41ebe3bc3704d863e634846cbaafcd8bf7 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 16 Dec 2019 17:54:47 +0000 Subject: [PATCH 1/5] Create InboundSmsHistory model and table --- app/models.py | 12 ++++++ .../versions/0311_add_inbound_sms_history.py | 37 +++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 migrations/versions/0311_add_inbound_sms_history.py diff --git a/app/models.py b/app/models.py index 06335c6d5..9f99767f5 100644 --- a/app/models.py +++ b/app/models.py @@ -1898,6 +1898,18 @@ class InboundSms(db.Model): } +class InboundSmsHistory(db.Model, HistoryModel): + __tablename__ = 'inbound_sms_history' + id = db.Column(UUID(as_uuid=True), primary_key=True) + created_at = db.Column(db.DateTime, nullable=False) + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False) + service = db.relationship('Service', backref='inbound_sms_history') # what does this one do? + notify_number = db.Column(db.String, nullable=False) + provider_date = db.Column(db.DateTime) + provider_reference = db.Column(db.String) + provider = db.Column(db.String, nullable=False) + + class LetterRate(db.Model): __tablename__ = 'letter_rates' diff --git a/migrations/versions/0311_add_inbound_sms_history.py b/migrations/versions/0311_add_inbound_sms_history.py new file mode 100644 index 000000000..7a70e1cd5 --- /dev/null +++ b/migrations/versions/0311_add_inbound_sms_history.py @@ -0,0 +1,37 @@ +""" + +Revision ID: 0311_add_inbound_sms_history +Revises: 0310_returned_letters_table +Create Date: 2019-12-17 11:02:17.572689 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0311_add_inbound_sms_history' +down_revision = '0310_returned_letters_table' + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('inbound_sms_history', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=True), + sa.Column('notify_number', sa.String(), nullable=False), + sa.Column('provider_date', sa.DateTime(), nullable=True), + sa.Column('provider_reference', sa.String(), nullable=True), + sa.Column('provider', sa.String(), nullable=False), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_index(op.f('ix_inbound_sms_history_service_id'), 'inbound_sms_history', ['service_id'], unique=False) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_index(op.f('ix_inbound_sms_history_service_id'), table_name='inbound_sms_history') + op.drop_table('inbound_sms_history') + # ### end Alembic commands ### From a6b4675ae7f66dfe557c89230ec52d7a5ad90aa6 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 17 Dec 2019 18:04:22 +0000 Subject: [PATCH 2/5] Populate inbound sms history when deleting inbound sms --- app/dao/inbound_sms_dao.py | 66 +++++++++++++++++++++-- tests/app/dao/test_inbound_sms_dao.py | 77 +++++++++++++++++++++++++-- 2 files changed, 136 insertions(+), 7 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index d7e6f86e0..301a3c966 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -2,10 +2,11 @@ from flask import current_app from notifications_utils.statsd_decorators import statsd from sqlalchemy import desc, and_ from sqlalchemy.orm import aliased +from sqlalchemy.dialects.postgresql import insert from app import db from app.dao.dao_utils import transactional -from app.models import InboundSms, Service, ServiceDataRetention, SMS_TYPE +from app.models import InboundSms, InboundSmsHistory, Service, ServiceDataRetention, SMS_TYPE from app.utils import midnight_n_days_ago @@ -78,6 +79,33 @@ def _delete_inbound_sms(datetime_to_delete_from, query_filter): # set to nonzero just to enter the loop number_deleted = 1 while number_deleted > 0: + + offset = 0 + inbound_sms_query = db.session.query( + *[x.name for x in InboundSmsHistory.__table__.c] + ).filter(InboundSms.id.in_(subquery)) + inbound_sms_count = inbound_sms_query.count() + + while offset < inbound_sms_count: + statement = insert(InboundSmsHistory).from_select( + InboundSmsHistory.__table__.c, + inbound_sms_query.limit(query_limit).offset(offset) + ) + + statement = statement.on_conflict_do_update( + constraint="inbound_sms_history_pkey", + set_={ + "created_at": statement.excluded.created_at, + "service_id": statement.excluded.service_id, + "notify_number": statement.excluded.notify_number, + "provider_date": statement.excluded.provider_date, + "provider_reference": statement.excluded.provider_reference, + "provider": statement.excluded.provider + } + ) + db.session.connection().execute(statement) + + offset += query_limit number_deleted = InboundSms.query.filter(InboundSms.id.in_(subquery)).delete(synchronize_session='fetch') deleted += number_deleted @@ -95,8 +123,8 @@ def delete_inbound_sms_older_than_retention(): ).filter( ServiceDataRetention.notification_type == SMS_TYPE ).all() - deleted = 0 + for f in flexible_data_retention: n_days_ago = midnight_n_days_ago(f.days_of_retention) @@ -110,12 +138,42 @@ def delete_inbound_sms_older_than_retention(): deleted += _delete_inbound_sms(seven_days_ago, query_filter=[ InboundSms.service_id.notin_(x.service_id for x in flexible_data_retention), ]) - current_app.logger.info('Deleted {} inbound sms'.format(deleted)) - return deleted +def insert_update_inbound_sms_history(date_to_delete_from, service_id, query_limit=10000): + offset = 0 + inbound_sms_query = db.session.query( + *[x.name for x in InboundSmsHistory.__table__.c] + ).filter( + InboundSms.service_id == service_id, + InboundSms.created_at < date_to_delete_from, + ) + inbound_sms_count = inbound_sms_query.count() + + while offset < inbound_sms_count: + statement = insert(InboundSmsHistory).from_select( + InboundSmsHistory.__table__.c, + inbound_sms_query.limit(query_limit).offset(offset) + ) + + statement = statement.on_conflict_do_update( + constraint="inbound_sms_history_pkey", + set_={ + "created_at": statement.excluded.created_at, + "service_id": statement.excluded.service_id, + "notify_number": statement.excluded.notify_number, + "provider_date": statement.excluded.provider_date, + "provider_reference": statement.excluded.provider_reference, + "provider": statement.excluded.provider + } + ) + db.session.connection().execute(statement) + + offset += query_limit + + def dao_get_inbound_sms_by_id(service_id, inbound_id): return InboundSms.query.filter_by( id=inbound_id, diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index 684773358..ee3363f5b 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -9,8 +9,15 @@ from app.dao.inbound_sms_dao import ( delete_inbound_sms_older_than_retention, dao_get_inbound_sms_by_id, dao_get_paginated_inbound_sms_for_service_for_public_api, - dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service + dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service, + db, + insert_update_inbound_sms_history ) + +from app.models import InboundSmsHistory + +from app.utils import midnight_n_days_ago + from tests.conftest import set_config from tests.app.db import create_inbound_sms, create_service, create_service_data_retention @@ -90,7 +97,7 @@ def test_count_inbound_sms_for_service_filters_messages_older_than_n_days(sample @freeze_time("2017-06-08 12:00:00") -def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session): +def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session, mocker): no_retention_service = create_service(service_name='no retention') short_retention_service = create_service(service_name='three days') long_retention_service = create_service(service_name='thirty days') @@ -99,7 +106,6 @@ def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session create_service_data_retention(long_retention_service, notification_type='sms', days_of_retention=30) create_service_data_retention(short_retention_service, notification_type='sms', days_of_retention=3) - # email retention doesn't affect anything create_service_data_retention(short_retention_service, notification_type='email', days_of_retention=4) dates = [ @@ -115,6 +121,9 @@ def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session deleted_count = delete_inbound_sms_older_than_retention() + history = InboundSmsHistory.query.all() + assert len(history) == 7 + # four deleted for the 3-day service, two for the default seven days one, one for the 30 day assert deleted_count == 7 assert { @@ -272,3 +281,65 @@ def test_most_recent_inbound_sms_only_returns_values_within_7_days(sample_servic assert len(res.items) == 1 assert res.items[0].content == 'new' + + +def test_insert_update_inbound_sms_history_limits_by_query_by_date(sample_service): + # becoming history + inbound_1 = create_inbound_sms( + sample_service, user_number='1', content='old', created_at=datetime(2017, 4, 2, 22, 59, 59) + ) + # not becoming history yet + create_inbound_sms(sample_service, user_number='2', content='new', created_at=datetime(2017, 4, 2, 23, 0, 0)) + + with freeze_time('Monday 10th April 2017 12:00:00'): + insert_update_inbound_sms_history( + date_to_delete_from=midnight_n_days_ago(7), + service_id=sample_service.id) + history = InboundSmsHistory.query.all() + assert len(history) == 1 + + assert history[0].id == inbound_1.id + + +def test_insert_update_inbound_sms_history_above_query_limit(sample_service, mocker): + # becoming history + create_inbound_sms( + sample_service, user_number='1', content='old', created_at=datetime(2017, 4, 2, 22, 59, 59) + ) + create_inbound_sms( + sample_service, user_number='1', content='old', created_at=datetime(2017, 4, 2, 20, 59, 59) + ) + # not becoming history yet + create_inbound_sms(sample_service, user_number='2', content='new', created_at=datetime(2017, 4, 2, 23, 0, 0)) + + db_connection_spy = mocker.spy(db.session, 'connection') + db_commit_spy = mocker.spy(db.session, 'commit') + + with freeze_time('Monday 10th April 2017 12:00:00'): + insert_update_inbound_sms_history( + date_to_delete_from=midnight_n_days_ago(7), + service_id=sample_service.id, + query_limit=1 + ) + history = InboundSmsHistory.query.all() + assert len(history) == 2 + + assert db_connection_spy.call_count == 2 + assert db_commit_spy.call_count == 2 + + +def test_insert_update_inbound_sms_history_only_updates_given_service(sample_service): + inbound_1 = create_inbound_sms( + sample_service, user_number='1', content='old', created_at=datetime(2017, 4, 2, 22, 59, 59) + ) + other_service = create_service(service_name='another service') + create_inbound_sms(other_service, user_number='2', content='new', created_at=datetime(2017, 4, 2, 22, 0, 0)) + + with freeze_time('Monday 10th April 2017 12:00:00'): + insert_update_inbound_sms_history( + date_to_delete_from=midnight_n_days_ago(7), + service_id=sample_service.id) + history = InboundSmsHistory.query.all() + assert len(history) == 1 + + assert history[0].id == inbound_1.id From 448cd1e94e28a774834e57d78cdf95ef1236dfa9 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 20 Dec 2019 11:19:36 +0000 Subject: [PATCH 3/5] Integrate inbound history insert into delete inbound sms function --- app/dao/inbound_sms_dao.py | 143 +++++++++++--------------- tests/app/dao/test_inbound_sms_dao.py | 66 ------------ 2 files changed, 59 insertions(+), 150 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 301a3c966..abfce6740 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -63,93 +63,11 @@ def dao_count_inbound_sms_for_service(service_id, limit_days): ).count() -def _delete_inbound_sms(datetime_to_delete_from, query_filter): - query_limit = 10000 - - subquery = db.session.query( - InboundSms.id - ).filter( - InboundSms.created_at < datetime_to_delete_from, - *query_filter - ).limit( - query_limit - ).subquery() - - deleted = 0 - # set to nonzero just to enter the loop - number_deleted = 1 - while number_deleted > 0: - - offset = 0 - inbound_sms_query = db.session.query( - *[x.name for x in InboundSmsHistory.__table__.c] - ).filter(InboundSms.id.in_(subquery)) - inbound_sms_count = inbound_sms_query.count() - - while offset < inbound_sms_count: - statement = insert(InboundSmsHistory).from_select( - InboundSmsHistory.__table__.c, - inbound_sms_query.limit(query_limit).offset(offset) - ) - - statement = statement.on_conflict_do_update( - constraint="inbound_sms_history_pkey", - set_={ - "created_at": statement.excluded.created_at, - "service_id": statement.excluded.service_id, - "notify_number": statement.excluded.notify_number, - "provider_date": statement.excluded.provider_date, - "provider_reference": statement.excluded.provider_reference, - "provider": statement.excluded.provider - } - ) - db.session.connection().execute(statement) - - offset += query_limit - number_deleted = InboundSms.query.filter(InboundSms.id.in_(subquery)).delete(synchronize_session='fetch') - deleted += number_deleted - - return deleted - - -@statsd(namespace="dao") -@transactional -def delete_inbound_sms_older_than_retention(): - current_app.logger.info('Deleting inbound sms for services with flexible data retention') - - flexible_data_retention = ServiceDataRetention.query.join( - ServiceDataRetention.service, - Service.inbound_number - ).filter( - ServiceDataRetention.notification_type == SMS_TYPE - ).all() - deleted = 0 - - for f in flexible_data_retention: - n_days_ago = midnight_n_days_ago(f.days_of_retention) - - current_app.logger.info("Deleting inbound sms for service id: {}".format(f.service_id)) - deleted += _delete_inbound_sms(n_days_ago, query_filter=[InboundSms.service_id == f.service_id]) - - current_app.logger.info('Deleting inbound sms for services without flexible data retention') - - seven_days_ago = midnight_n_days_ago(7) - - deleted += _delete_inbound_sms(seven_days_ago, query_filter=[ - InboundSms.service_id.notin_(x.service_id for x in flexible_data_retention), - ]) - current_app.logger.info('Deleted {} inbound sms'.format(deleted)) - return deleted - - -def insert_update_inbound_sms_history(date_to_delete_from, service_id, query_limit=10000): +def _insert_update_notification_history(subquery, query_limit=10000): offset = 0 inbound_sms_query = db.session.query( *[x.name for x in InboundSmsHistory.__table__.c] - ).filter( - InboundSms.service_id == service_id, - InboundSms.created_at < date_to_delete_from, - ) + ).filter(InboundSms.id.in_(subquery)) inbound_sms_count = inbound_sms_query.count() while offset < inbound_sms_count: @@ -174,6 +92,63 @@ def insert_update_inbound_sms_history(date_to_delete_from, service_id, query_lim offset += query_limit +def _delete_inbound_sms(datetime_to_delete_from, query_filter): + query_limit = 10000 + + subquery = db.session.query( + InboundSms.id + ).filter( + InboundSms.created_at < datetime_to_delete_from, + *query_filter + ).limit( + query_limit + ).subquery() + + deleted = 0 + # set to nonzero just to enter the loop + number_deleted = 1 + while number_deleted > 0: + _insert_update_notification_history(subquery, query_limit=query_limit) + + number_deleted = InboundSms.query.filter(InboundSms.id.in_(subquery)).delete(synchronize_session='fetch') + deleted += number_deleted + + return deleted + + +@statsd(namespace="dao") +@transactional +def delete_inbound_sms_older_than_retention(): + current_app.logger.info('Deleting inbound sms for services with flexible data retention') + + flexible_data_retention = ServiceDataRetention.query.join( + ServiceDataRetention.service, + Service.inbound_number + ).filter( + ServiceDataRetention.notification_type == SMS_TYPE + ).all() + + deleted = 0 + + for f in flexible_data_retention: + n_days_ago = midnight_n_days_ago(f.days_of_retention) + + current_app.logger.info("Deleting inbound sms for service id: {}".format(f.service_id)) + deleted += _delete_inbound_sms(n_days_ago, query_filter=[InboundSms.service_id == f.service_id]) + + current_app.logger.info('Deleting inbound sms for services without flexible data retention') + + seven_days_ago = midnight_n_days_ago(7) + + deleted += _delete_inbound_sms(seven_days_ago, query_filter=[ + InboundSms.service_id.notin_(x.service_id for x in flexible_data_retention), + ]) + + current_app.logger.info('Deleted {} inbound sms'.format(deleted)) + + return deleted + + def dao_get_inbound_sms_by_id(service_id, inbound_id): return InboundSms.query.filter_by( id=inbound_id, diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index ee3363f5b..d465e7174 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -10,14 +10,10 @@ from app.dao.inbound_sms_dao import ( dao_get_inbound_sms_by_id, dao_get_paginated_inbound_sms_for_service_for_public_api, dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service, - db, - insert_update_inbound_sms_history ) from app.models import InboundSmsHistory -from app.utils import midnight_n_days_ago - from tests.conftest import set_config from tests.app.db import create_inbound_sms, create_service, create_service_data_retention @@ -281,65 +277,3 @@ def test_most_recent_inbound_sms_only_returns_values_within_7_days(sample_servic assert len(res.items) == 1 assert res.items[0].content == 'new' - - -def test_insert_update_inbound_sms_history_limits_by_query_by_date(sample_service): - # becoming history - inbound_1 = create_inbound_sms( - sample_service, user_number='1', content='old', created_at=datetime(2017, 4, 2, 22, 59, 59) - ) - # not becoming history yet - create_inbound_sms(sample_service, user_number='2', content='new', created_at=datetime(2017, 4, 2, 23, 0, 0)) - - with freeze_time('Monday 10th April 2017 12:00:00'): - insert_update_inbound_sms_history( - date_to_delete_from=midnight_n_days_ago(7), - service_id=sample_service.id) - history = InboundSmsHistory.query.all() - assert len(history) == 1 - - assert history[0].id == inbound_1.id - - -def test_insert_update_inbound_sms_history_above_query_limit(sample_service, mocker): - # becoming history - create_inbound_sms( - sample_service, user_number='1', content='old', created_at=datetime(2017, 4, 2, 22, 59, 59) - ) - create_inbound_sms( - sample_service, user_number='1', content='old', created_at=datetime(2017, 4, 2, 20, 59, 59) - ) - # not becoming history yet - create_inbound_sms(sample_service, user_number='2', content='new', created_at=datetime(2017, 4, 2, 23, 0, 0)) - - db_connection_spy = mocker.spy(db.session, 'connection') - db_commit_spy = mocker.spy(db.session, 'commit') - - with freeze_time('Monday 10th April 2017 12:00:00'): - insert_update_inbound_sms_history( - date_to_delete_from=midnight_n_days_ago(7), - service_id=sample_service.id, - query_limit=1 - ) - history = InboundSmsHistory.query.all() - assert len(history) == 2 - - assert db_connection_spy.call_count == 2 - assert db_commit_spy.call_count == 2 - - -def test_insert_update_inbound_sms_history_only_updates_given_service(sample_service): - inbound_1 = create_inbound_sms( - sample_service, user_number='1', content='old', created_at=datetime(2017, 4, 2, 22, 59, 59) - ) - other_service = create_service(service_name='another service') - create_inbound_sms(other_service, user_number='2', content='new', created_at=datetime(2017, 4, 2, 22, 0, 0)) - - with freeze_time('Monday 10th April 2017 12:00:00'): - insert_update_inbound_sms_history( - date_to_delete_from=midnight_n_days_ago(7), - service_id=sample_service.id) - history = InboundSmsHistory.query.all() - assert len(history) == 1 - - assert history[0].id == inbound_1.id From f8ff2d121f907a64a2aca2d504a965b86069e6f6 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 20 Dec 2019 12:34:59 +0000 Subject: [PATCH 4/5] Changes following review: - Check if right keys in new history rows - Improve model and get rid of old revision version - Add updated migration file - Test data when inserting into inbound sms history --- app/dao/inbound_sms_dao.py | 5 ++- app/models.py | 4 +-- .../versions/0311_add_inbound_sms_history.py | 6 +--- tests/app/dao/test_inbound_sms_dao.py | 32 ++++++++++++++++++- 4 files changed, 36 insertions(+), 11 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index abfce6740..40ef356b2 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -63,7 +63,7 @@ def dao_count_inbound_sms_for_service(service_id, limit_days): ).count() -def _insert_update_notification_history(subquery, query_limit=10000): +def _insert_update_inbound_sms_history(subquery, query_limit=10000): offset = 0 inbound_sms_query = db.session.query( *[x.name for x in InboundSmsHistory.__table__.c] @@ -79,7 +79,6 @@ def _insert_update_notification_history(subquery, query_limit=10000): statement = statement.on_conflict_do_update( constraint="inbound_sms_history_pkey", set_={ - "created_at": statement.excluded.created_at, "service_id": statement.excluded.service_id, "notify_number": statement.excluded.notify_number, "provider_date": statement.excluded.provider_date, @@ -108,7 +107,7 @@ def _delete_inbound_sms(datetime_to_delete_from, query_filter): # set to nonzero just to enter the loop number_deleted = 1 while number_deleted > 0: - _insert_update_notification_history(subquery, query_limit=query_limit) + _insert_update_inbound_sms_history(subquery, query_limit=query_limit) number_deleted = InboundSms.query.filter(InboundSms.id.in_(subquery)).delete(synchronize_session='fetch') deleted += number_deleted diff --git a/app/models.py b/app/models.py index 9f99767f5..f8a2521a9 100644 --- a/app/models.py +++ b/app/models.py @@ -1901,9 +1901,9 @@ class InboundSms(db.Model): class InboundSmsHistory(db.Model, HistoryModel): __tablename__ = 'inbound_sms_history' id = db.Column(UUID(as_uuid=True), primary_key=True) - created_at = db.Column(db.DateTime, nullable=False) + created_at = db.Column(db.DateTime, index=True, unique=False, nullable=False) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False) - service = db.relationship('Service', backref='inbound_sms_history') # what does this one do? + service = db.relationship('Service') notify_number = db.Column(db.String, nullable=False) provider_date = db.Column(db.DateTime) provider_reference = db.Column(db.String) diff --git a/migrations/versions/0311_add_inbound_sms_history.py b/migrations/versions/0311_add_inbound_sms_history.py index 7a70e1cd5..44def8d55 100644 --- a/migrations/versions/0311_add_inbound_sms_history.py +++ b/migrations/versions/0311_add_inbound_sms_history.py @@ -2,7 +2,7 @@ Revision ID: 0311_add_inbound_sms_history Revises: 0310_returned_letters_table -Create Date: 2019-12-17 11:02:17.572689 +Create Date: 2019-12-20 15:38:53.358509 """ from alembic import op @@ -14,7 +14,6 @@ down_revision = '0310_returned_letters_table' def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.create_table('inbound_sms_history', sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), sa.Column('created_at', sa.DateTime(), nullable=False), @@ -27,11 +26,8 @@ def upgrade(): sa.PrimaryKeyConstraint('id') ) op.create_index(op.f('ix_inbound_sms_history_service_id'), 'inbound_sms_history', ['service_id'], unique=False) - # ### end Alembic commands ### def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.drop_index(op.f('ix_inbound_sms_history_service_id'), table_name='inbound_sms_history') op.drop_table('inbound_sms_history') - # ### end Alembic commands ### diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index d465e7174..cd8daea7f 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -93,7 +93,7 @@ def test_count_inbound_sms_for_service_filters_messages_older_than_n_days(sample @freeze_time("2017-06-08 12:00:00") -def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session, mocker): +def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session): no_retention_service = create_service(service_name='no retention') short_retention_service = create_service(service_name='three days') long_retention_service = create_service(service_name='thirty days') @@ -133,6 +133,36 @@ def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session } == set(dates[:4]) +@freeze_time("2019-12-20 12:00:00") +def test_insert_into_inbound_sms_history_when_deleting_inbound_sms(sample_service): + create_inbound_sms( + sample_service, created_at=datetime(2019, 12, 12, 20, 20), + notify_number='07700900100', + provider_date=datetime(2019, 12, 12, 20, 19), + provider_reference='from daisy pie', + provider='unicorn' + ) + create_inbound_sms(sample_service, created_at=datetime(2019, 12, 19, 20, 19)) + + delete_inbound_sms_older_than_retention() + history = InboundSmsHistory.query.all() + assert len(history) == 1 + + for key_name in [ + 'provider', 'provider_date', 'service_id', 'created_at', 'provider_reference', 'notify_number', 'id' + ]: + assert key_name in vars(history[0]) + + for key_name in ['content', 'user_number']: + assert key_name not in vars(history[0]) + + assert history[0].notify_number == '07700900100' + assert history[0].provider_date == datetime(2019, 12, 12, 20, 19) + assert history[0].provider_reference == 'from daisy pie' + assert history[0].provider == 'unicorn' + assert history[0].created_at == datetime(2019, 12, 12, 20, 20) + + def test_get_inbound_sms_by_id_returns(sample_service): inbound_sms = create_inbound_sms(service=sample_service) inbound_from_db = dao_get_inbound_sms_by_id(inbound_sms.service.id, inbound_sms.id) From f948555ca8eab0229619aa199cd59f70ddc2f643 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Tue, 24 Dec 2019 09:29:27 +0000 Subject: [PATCH 5/5] Do nothing on db conflict For notification and notification_history we do an upsert. Here, as the inbound_sms table is never updated, only inserted to once (signified by lack of updated_at field), an upsert would be unnecessary. Therefore, if for some reason the delete statement failed as part of moving data into the inbound_sms_history table, we can simply just ignore any db conflicts raised by a rerun of `delete_inbound_sms_older_than_retention`. --- app/dao/inbound_sms_dao.py | 15 +++------- tests/app/dao/test_inbound_sms_dao.py | 40 +++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 40ef356b2..ba17ddd26 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -63,7 +63,7 @@ def dao_count_inbound_sms_for_service(service_id, limit_days): ).count() -def _insert_update_inbound_sms_history(subquery, query_limit=10000): +def _insert_inbound_sms_history(subquery, query_limit=10000): offset = 0 inbound_sms_query = db.session.query( *[x.name for x in InboundSmsHistory.__table__.c] @@ -76,15 +76,8 @@ def _insert_update_inbound_sms_history(subquery, query_limit=10000): inbound_sms_query.limit(query_limit).offset(offset) ) - statement = statement.on_conflict_do_update( - constraint="inbound_sms_history_pkey", - set_={ - "service_id": statement.excluded.service_id, - "notify_number": statement.excluded.notify_number, - "provider_date": statement.excluded.provider_date, - "provider_reference": statement.excluded.provider_reference, - "provider": statement.excluded.provider - } + statement = statement.on_conflict_do_nothing( + constraint="inbound_sms_history_pkey" ) db.session.connection().execute(statement) @@ -107,7 +100,7 @@ def _delete_inbound_sms(datetime_to_delete_from, query_filter): # set to nonzero just to enter the loop number_deleted = 1 while number_deleted > 0: - _insert_update_inbound_sms_history(subquery, query_limit=query_limit) + _insert_inbound_sms_history(subquery, query_limit=query_limit) number_deleted = InboundSms.query.filter(InboundSms.id.in_(subquery)).delete(synchronize_session='fetch') deleted += number_deleted diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index cd8daea7f..b3b3f00db 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -13,6 +13,7 @@ from app.dao.inbound_sms_dao import ( ) from app.models import InboundSmsHistory +from app import db from tests.conftest import set_config from tests.app.db import create_inbound_sms, create_service, create_service_data_retention @@ -163,6 +164,45 @@ def test_insert_into_inbound_sms_history_when_deleting_inbound_sms(sample_servic assert history[0].created_at == datetime(2019, 12, 12, 20, 20) +@freeze_time("2019-12-20 12:00:00") +def test_delete_inbound_sms_older_than_retention_does_nothing_when_database_conflict_raised(sample_service): + inbound_sms = create_inbound_sms( + sample_service, created_at=datetime(2019, 12, 12, 20, 20), + notify_number='07700900100', + provider_date=datetime(2019, 12, 12, 20, 19), + provider_reference='from daisy pie', + provider='unicorn' + ) + inbound_sms_id = inbound_sms.id + + # Insert data directly in to inbound_sms_history to mimic if we had run `delete_inbound_sms_older_than_retention` + # before but for some reason the delete statement had failed + conflict_creating_row = InboundSmsHistory( + id=inbound_sms.id, + service_id=inbound_sms.service.id, + created_at=inbound_sms.created_at, + notify_number=inbound_sms.notify_number, + provider_date=inbound_sms.provider_date, + provider_reference=inbound_sms.provider_reference, + provider=inbound_sms.provider, + ) + db.session.add(conflict_creating_row) + db.session.commit() + assert conflict_creating_row.id + + delete_inbound_sms_older_than_retention() + + history = InboundSmsHistory.query.all() + assert len(history) == 1 + + assert history[0].id == inbound_sms_id + assert history[0].notify_number == '07700900100' + assert history[0].provider_date == datetime(2019, 12, 12, 20, 19) + assert history[0].provider_reference == 'from daisy pie' + assert history[0].provider == 'unicorn' + assert history[0].created_at == datetime(2019, 12, 12, 20, 20) + + def test_get_inbound_sms_by_id_returns(sample_service): inbound_sms = create_inbound_sms(service=sample_service) inbound_from_db = dao_get_inbound_sms_by_id(inbound_sms.service.id, inbound_sms.id)