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

@@ -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

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)

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 ###

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)