diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index d7e6f86e0..ba17ddd26 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 @@ -62,6 +63,27 @@ def dao_count_inbound_sms_for_service(service_id, limit_days): ).count() +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] + ).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_nothing( + constraint="inbound_sms_history_pkey" + ) + db.session.connection().execute(statement) + + offset += query_limit + + def _delete_inbound_sms(datetime_to_delete_from, query_filter): query_limit = 10000 @@ -78,6 +100,8 @@ 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_inbound_sms_history(subquery, query_limit=query_limit) + number_deleted = InboundSms.query.filter(InboundSms.id.in_(subquery)).delete(synchronize_session='fetch') deleted += number_deleted @@ -97,6 +121,7 @@ def delete_inbound_sms_older_than_retention(): ).all() deleted = 0 + for f in flexible_data_retention: n_days_ago = midnight_n_days_ago(f.days_of_retention) diff --git a/app/models.py b/app/models.py index 06335c6d5..f8a2521a9 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, 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') + 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..44def8d55 --- /dev/null +++ b/migrations/versions/0311_add_inbound_sms_history.py @@ -0,0 +1,33 @@ +""" + +Revision ID: 0311_add_inbound_sms_history +Revises: 0310_returned_letters_table +Create Date: 2019-12-20 15:38:53.358509 + +""" +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(): + 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) + + +def downgrade(): + op.drop_index(op.f('ix_inbound_sms_history_service_id'), table_name='inbound_sms_history') + op.drop_table('inbound_sms_history') diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index 684773358..b3b3f00db 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -9,8 +9,12 @@ 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, ) + +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 @@ -99,7 +103,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 +118,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 { @@ -128,6 +134,75 @@ 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) + + +@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)