From e80a002c5888a7be7fa434995d176250aaca7246 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 9 Dec 2019 12:19:18 +0000 Subject: [PATCH 1/8] New table returned-letters The table will contain notification ids for services that have returned letters. This will make it easy to query the data in Notification_history since we can join on the primary key. --- app/dao/returned_letters_dao.py | 0 app/models.py | 14 +++++++-- .../versions/0310_returned_letters_table_.py | 30 +++++++++++++++++++ tests/app/dao/test_returned_letters_dao.py | 0 4 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 app/dao/returned_letters_dao.py create mode 100644 migrations/versions/0310_returned_letters_table_.py create mode 100644 tests/app/dao/test_returned_letters_dao.py diff --git a/app/dao/returned_letters_dao.py b/app/dao/returned_letters_dao.py new file mode 100644 index 000000000..e69de29bb diff --git a/app/models.py b/app/models.py index 682e8bd83..a7ece9dbf 100644 --- a/app/models.py +++ b/app/models.py @@ -2018,8 +2018,7 @@ class Complaint(db.Model): __tablename__ = 'complaints' id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - notification_id = db.Column(UUID(as_uuid=True), db.ForeignKey('notification_history.id'), - index=True, nullable=False) + notification_id = db.Column(UUID(as_uuid=True), index=True, nullable=False) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), unique=False, index=True, nullable=False) service = db.relationship(Service, backref=db.backref('complaints')) ses_feedback_id = db.Column(db.Text, nullable=True) @@ -2071,3 +2070,14 @@ class ServiceDataRetention(db.Model): "created_at": self.created_at.strftime(DATETIME_FORMAT), "updated_at": self.updated_at.strftime(DATETIME_FORMAT) if self.updated_at else None, } + + +class ReturnedLetter(db.Model): + __tablename__ = 'returned_letters' + + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + reported_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), unique=False, index=True, nullable=False) + service = db.relationship(Service, backref=db.backref('returned_letters')) + notification_id = db.Column(UUID(as_uuid=True), unique=True, nullable=False) + diff --git a/migrations/versions/0310_returned_letters_table_.py b/migrations/versions/0310_returned_letters_table_.py new file mode 100644 index 000000000..a33be2595 --- /dev/null +++ b/migrations/versions/0310_returned_letters_table_.py @@ -0,0 +1,30 @@ +""" + +Revision ID: 0310_returned_letters_table +Revises: 0309_add_uq_key_row_number +Create Date: 2019-12-09 12:13:49.432993 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0310_returned_letters_table' +down_revision = '0309_add_uq_key_row_number' + + +def upgrade(): + op.create_table('returned_letters', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('reported_at', sa.DateTime(), nullable=False), + sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('notification_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), + sa.PrimaryKeyConstraint('id'), + sa.UniqueConstraint('notification_id') + ) + op.create_index(op.f('ix_returned_letters_service_id'), 'returned_letters', ['service_id'], unique=False) + + +def downgrade(): + op.drop_table('returned_letters') diff --git a/tests/app/dao/test_returned_letters_dao.py b/tests/app/dao/test_returned_letters_dao.py new file mode 100644 index 000000000..e69de29bb From 15762d5c2248e5b4e8217c5940092735fef8667c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 9 Dec 2019 16:19:46 +0000 Subject: [PATCH 2/8] Method to insert or update the returned-letters --- app/dao/returned_letters_dao.py | 40 +++++++++++ tests/app/dao/test_returned_letters_dao.py | 83 ++++++++++++++++++++++ 2 files changed, 123 insertions(+) diff --git a/app/dao/returned_letters_dao.py b/app/dao/returned_letters_dao.py index e69de29bb..24d8ae438 100644 --- a/app/dao/returned_letters_dao.py +++ b/app/dao/returned_letters_dao.py @@ -0,0 +1,40 @@ +from datetime import datetime + +from sqlalchemy.dialects.postgresql import insert + +from app import db +from app.dao.dao_utils import transactional +from app.models import Notification, NotificationHistory, ReturnedLetter + + +@transactional +def insert_or_update_returned_letters(references): + data = _get_notification_ids_for_references(references) + now = datetime.utcnow() + for row in data: + table = ReturnedLetter.__table__ + + stmt = insert(table).values( + reported_at=now, + service_id=row.service_id, + notification_id=row.id) + + stmt = stmt.on_conflict_do_update( + index_elements=[table.c.notification_id], + set_={ + 'reported_at': now, + } + ) + db.session.connection().execute(stmt) + + +def _get_notification_ids_for_references(references): + notification_ids = db.session.query(Notification.id, Notification.service_id).filter( + Notification.reference.in_(references) + ).all() + + notification_history_ids = db.session.query(NotificationHistory.id, NotificationHistory.service_id).filter( + NotificationHistory.reference.in_(references) + ).all() + + return notification_ids + notification_history_ids diff --git a/tests/app/dao/test_returned_letters_dao.py b/tests/app/dao/test_returned_letters_dao.py index e69de29bb..1a94f1fde 100644 --- a/tests/app/dao/test_returned_letters_dao.py +++ b/tests/app/dao/test_returned_letters_dao.py @@ -0,0 +1,83 @@ +from datetime import datetime + +from freezegun import freeze_time + +from app.dao.returned_letters_dao import insert_or_update_returned_letters +from app.models import ReturnedLetter +from tests.app.db import create_notification, create_notification_history + + +def test_insert_or_update_returned_letters_inserts(sample_letter_template): + notification = create_notification(template=sample_letter_template, + reference='ref1') + history = create_notification_history(template=sample_letter_template, + reference='ref2') + + assert ReturnedLetter.query.count() == 0 + + insert_or_update_returned_letters(['ref1', 'ref2']) + + returned_letters = ReturnedLetter.query.all() + + assert len(returned_letters) == 2 + returned_letters_ = [x.notification_id for x in returned_letters] + assert notification.id in returned_letters_ + assert history.id in returned_letters_ + + +def test_insert_or_update_returned_letters_updates(sample_letter_template): + notification = create_notification(template=sample_letter_template, + reference='ref1') + history = create_notification_history(template=sample_letter_template, + reference='ref2') + + assert ReturnedLetter.query.count() == 0 + with freeze_time('2019-12-09 13:30'): + insert_or_update_returned_letters(['ref1', 'ref2']) + returned_letters = ReturnedLetter.query.all() + assert len(returned_letters) == 2 + for x in returned_letters: + assert x.reported_at == datetime(2019, 12, 9, 13, 30) + assert x.notification_id in [notification.id, history.id] + + with freeze_time('2019-12-10 14:20'): + insert_or_update_returned_letters(['ref1', 'ref2']) + + returned_letters = ReturnedLetter.query.all() + assert len(returned_letters) == 2 + for x in returned_letters: + assert x.reported_at == datetime(2019, 12, 10, 14, 20) + assert x.notification_id in [notification.id, history.id] + + +def test_insert_or_update_returned_letters_when_no_notification(sample_letter_template): + insert_or_update_returned_letters(['ref1']) + assert ReturnedLetter.query.count() == 0 + + +def test_insert_or_update_returned_letters_for_history_only(sample_letter_template): + history_1 = create_notification_history(template=sample_letter_template, + reference='ref1') + history_2 = create_notification_history(template=sample_letter_template, + reference='ref2') + + assert ReturnedLetter.query.count() == 0 + insert_or_update_returned_letters(['ref1', 'ref2']) + returned_letters = ReturnedLetter.query.all() + assert len(returned_letters) == 2 + for x in returned_letters: + assert x.notification_id in [history_1.id, history_2.id] + + +def test_insert_or_update_returned_letters_with_duplicates_in_reference_list(sample_letter_template): + notification_1 = create_notification(template=sample_letter_template, + reference='ref1') + notification_2 = create_notification(template=sample_letter_template, + reference='ref2') + + assert ReturnedLetter.query.count() == 0 + insert_or_update_returned_letters(['ref1', 'ref2', 'ref1', 'ref2']) + returned_letters = ReturnedLetter.query.all() + assert len(returned_letters) == 2 + for x in returned_letters: + assert x.notification_id in [notification_1.id, notification_2.id] From c8368d908b7f1de10f716fa06e4ac2cf5f3f34f7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 9 Dec 2019 16:23:09 +0000 Subject: [PATCH 3/8] Update process_returned_letters task to insert or update the returned_letter table. --- app/celery/tasks.py | 3 +++ tests/app/celery/test_tasks.py | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index d03d08e43..616d9ee77 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -43,6 +43,7 @@ from app.dao.notifications_dao import ( dao_get_notification_history_by_reference, ) from app.dao.provider_details_dao import get_provider_details_by_notification_type +from app.dao.returned_letters_dao import insert_or_update_returned_letters from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id @@ -619,6 +620,8 @@ def process_returned_letters_list(notification_references): {"status": NOTIFICATION_RETURNED_LETTER} ) + insert_or_update_returned_letters(notification_references) + current_app.logger.info( "Updated {} letter notifications ({} history notifications, from {} references) to returned-letter".format( updated, updated_history, len(notification_references) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 515ef5f8c..698e61e74 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -44,6 +44,7 @@ from app.models import ( JOB_STATUS_IN_PROGRESS, LETTER_TYPE, SMS_TYPE, + ReturnedLetter ) from tests.app import load_example_csv @@ -1626,3 +1627,15 @@ def test_process_returned_letters_list_updates_history_if_notification_is_alread assert [n.status for n in notifications] == ['returned-letter', 'returned-letter'] assert all(n.updated_at for n in notifications) + + +def test_process_returned_letters_populates_returned_letters_table( + sample_letter_template +): + create_notification_history(sample_letter_template, reference='ref1') + create_notification_history(sample_letter_template, reference='ref2') + + process_returned_letters_list(['ref1', 'ref2', 'unknown-ref']) + + returned_letters = ReturnedLetter.query.all() + assert len(returned_letters) == 2 From 40a0c6292690eda9cf53ff7c0bba4dfc645fa664 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 9 Dec 2019 17:27:18 +0000 Subject: [PATCH 4/8] New endpoint to return a summary of returned letters for the service. --- app/dao/returned_letters_dao.py | 33 +++++++++++++------ app/service/rest.py | 10 ++++++ tests/app/dao/test_returned_letters_dao.py | 37 ++++++++++++++++++++-- tests/app/db.py | 20 +++++++++++- 4 files changed, 87 insertions(+), 13 deletions(-) diff --git a/app/dao/returned_letters_dao.py b/app/dao/returned_letters_dao.py index 24d8ae438..48e121ce0 100644 --- a/app/dao/returned_letters_dao.py +++ b/app/dao/returned_letters_dao.py @@ -1,5 +1,6 @@ from datetime import datetime +from sqlalchemy import func, desc from sqlalchemy.dialects.postgresql import insert from app import db @@ -7,6 +8,19 @@ from app.dao.dao_utils import transactional from app.models import Notification, NotificationHistory, ReturnedLetter + +def _get_notification_ids_for_references(references): + notification_ids = db.session.query(Notification.id, Notification.service_id).filter( + Notification.reference.in_(references) + ).all() + + notification_history_ids = db.session.query(NotificationHistory.id, NotificationHistory.service_id).filter( + NotificationHistory.reference.in_(references) + ).all() + + return notification_ids + notification_history_ids + + @transactional def insert_or_update_returned_letters(references): data = _get_notification_ids_for_references(references) @@ -28,13 +42,14 @@ def insert_or_update_returned_letters(references): db.session.connection().execute(stmt) -def _get_notification_ids_for_references(references): - notification_ids = db.session.query(Notification.id, Notification.service_id).filter( - Notification.reference.in_(references) +def get_returned_letter_summary(service_id): + return db.session.query( + func.count(ReturnedLetter.notification_id).label('returned_letter_count'), + ReturnedLetter.reported_at + ).filter( + ReturnedLetter.service_id == service_id, + ).group_by( + ReturnedLetter.reported_at + ).order_by( + desc(ReturnedLetter.reported_at) ).all() - - notification_history_ids = db.session.query(NotificationHistory.id, NotificationHistory.service_id).filter( - NotificationHistory.reference.in_(references) - ).all() - - return notification_ids + notification_history_ids diff --git a/app/service/rest.py b/app/service/rest.py index 60141e87e..e34f90e26 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -29,6 +29,7 @@ from app.dao.fact_notification_status_dao import ( ) from app.dao.inbound_numbers_dao import dao_allocate_number_for_service from app.dao.organisation_dao import dao_get_organisation_by_service_id +from app.dao.returned_letters_dao import get_returned_letter_summary from app.dao.service_data_retention_dao import ( fetch_service_data_retention, fetch_service_data_retention_by_id, @@ -939,3 +940,12 @@ def check_if_reply_to_address_already_in_use(service_id, email_address): raise InvalidRequest( "Your service already uses ‘{}’ as an email reply-to address.".format(email_address), status_code=400 ) + + +@service_blueprint.route('/returned-letter-summary') +def returned_letter_summary(service_id): + results = get_returned_letter_summary(service_id) + + json_results = [{'returned_letter_count': x.returned_letter_count, 'reported_at': x.reported_at} for x in results] + + return jsonify(json_results) diff --git a/tests/app/dao/test_returned_letters_dao.py b/tests/app/dao/test_returned_letters_dao.py index 1a94f1fde..bb0836be0 100644 --- a/tests/app/dao/test_returned_letters_dao.py +++ b/tests/app/dao/test_returned_letters_dao.py @@ -1,10 +1,10 @@ -from datetime import datetime +from datetime import datetime, timedelta from freezegun import freeze_time -from app.dao.returned_letters_dao import insert_or_update_returned_letters +from app.dao.returned_letters_dao import insert_or_update_returned_letters, get_returned_letter_summary from app.models import ReturnedLetter -from tests.app.db import create_notification, create_notification_history +from tests.app.db import create_notification, create_notification_history, create_returned_letter def test_insert_or_update_returned_letters_inserts(sample_letter_template): @@ -81,3 +81,34 @@ def test_insert_or_update_returned_letters_with_duplicates_in_reference_list(sam assert len(returned_letters) == 2 for x in returned_letters: assert x.notification_id in [notification_1.id, notification_2.id] + + +def test_get_returned_letter_summary(sample_service): + now = datetime.utcnow() + create_returned_letter(sample_service, reported_at=now) + create_returned_letter(sample_service, reported_at=now) + + results = get_returned_letter_summary(sample_service.id) + + assert len(results) == 1 + + assert results[0].returned_letter_count == 2 + assert results[0].reported_at == now + + +def test_get_returned_letter_summary_orders_by_reported_at(sample_service): + now = datetime.utcnow() + last_month = datetime.utcnow() - timedelta(days=30) + create_returned_letter(sample_service, reported_at=now) + create_returned_letter(sample_service, reported_at=now) + create_returned_letter(sample_service, reported_at=now) + create_returned_letter(sample_service, reported_at=last_month) + create_returned_letter(sample_service, reported_at=last_month) + + results = get_returned_letter_summary(sample_service.id) + + assert len(results) == 2 + assert results[0].reported_at == now + assert results[0].returned_letter_count == 3 + assert results[1].reported_at == last_month + assert results[1].returned_letter_count == 2 diff --git a/tests/app/db.py b/tests/app/db.py index 4cbcfdfc2..50840bf2c 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -59,7 +59,9 @@ from app.models import ( TemplateFolder, LetterBranding, Domain, - NotificationHistory + NotificationHistory, + NOTIFICATION_RETURNED_LETTER, + ReturnedLetter ) @@ -940,3 +942,19 @@ def set_up_usage_data(start_date): notifications_sent=15, billable_unit=4, rate=.55, postage='second') return org, org_3, service, service_3, service_4, service_sms_only + + +def create_returned_letter(service, reported_at=None): + if not service: + service = create_service(service_name='a - with sms and letter') + template = create_template(service=service, template_type=LETTER_TYPE) + notification = create_notification(template=template, status=NOTIFICATION_RETURNED_LETTER) + returned_letter = ReturnedLetter( + service_id= service.id, + reported_at= reported_at or datetime.utcnow(), + notification_id= notification.id + ) + + db.session.add(returned_letter) + db.session.commit() + return returned_letter \ No newline at end of file From c8364b4dc46553b6b1fab160d2e23d08426d69e1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 10 Dec 2019 16:21:55 +0000 Subject: [PATCH 5/8] Add endpoint to return the summary data for returned letters. Returning the count of letters that are returned for each report date. --- app/dao/returned_letters_dao.py | 1 - app/models.py | 1 - app/service/rest.py | 2 +- tests/app/db.py | 8 ++++---- tests/app/service/test_rest.py | 11 ++++++++++- 5 files changed, 15 insertions(+), 8 deletions(-) diff --git a/app/dao/returned_letters_dao.py b/app/dao/returned_letters_dao.py index 48e121ce0..7e313178c 100644 --- a/app/dao/returned_letters_dao.py +++ b/app/dao/returned_letters_dao.py @@ -8,7 +8,6 @@ from app.dao.dao_utils import transactional from app.models import Notification, NotificationHistory, ReturnedLetter - def _get_notification_ids_for_references(references): notification_ids = db.session.query(Notification.id, Notification.service_id).filter( Notification.reference.in_(references) diff --git a/app/models.py b/app/models.py index a7ece9dbf..577beb64a 100644 --- a/app/models.py +++ b/app/models.py @@ -2080,4 +2080,3 @@ class ReturnedLetter(db.Model): service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), unique=False, index=True, nullable=False) service = db.relationship(Service, backref=db.backref('returned_letters')) notification_id = db.Column(UUID(as_uuid=True), unique=True, nullable=False) - diff --git a/app/service/rest.py b/app/service/rest.py index e34f90e26..4416a5a39 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -942,7 +942,7 @@ def check_if_reply_to_address_already_in_use(service_id, email_address): ) -@service_blueprint.route('/returned-letter-summary') +@service_blueprint.route('//returned-letter-summary', methods=['GET']) def returned_letter_summary(service_id): results = get_returned_letter_summary(service_id) diff --git a/tests/app/db.py b/tests/app/db.py index 50840bf2c..ac23654c2 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -950,11 +950,11 @@ def create_returned_letter(service, reported_at=None): template = create_template(service=service, template_type=LETTER_TYPE) notification = create_notification(template=template, status=NOTIFICATION_RETURNED_LETTER) returned_letter = ReturnedLetter( - service_id= service.id, - reported_at= reported_at or datetime.utcnow(), - notification_id= notification.id + service_id=service.id, + reported_at=reported_at or datetime.utcnow(), + notification_id=notification.id ) db.session.add(returned_letter) db.session.commit() - return returned_letter \ No newline at end of file + return returned_letter diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 5677428be..cd6bbae8f 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -47,7 +47,7 @@ from tests.app.db import ( create_domain, create_email_branding, create_annual_billing, -) + create_returned_letter) from tests.app.db import create_user @@ -3372,3 +3372,12 @@ def test_get_monthly_notification_data_by_service(mocker, admin_request): dao_mock.assert_called_once_with(start_date, end_date) assert response == [] + + +def test_get_returned_letter_summary(admin_request, sample_service): + create_returned_letter(sample_service, reported_at=datetime.utcnow()) + create_returned_letter(sample_service, reported_at=datetime.utcnow()-timedelta(days=3)) + + response = admin_request.get('service.returned_letter_summary', service_id=sample_service.id) + + assert len(response) == 2 \ No newline at end of file From 3908f2ad5933c3afce37aca7f0db65a2c89755b1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 10 Dec 2019 17:21:44 +0000 Subject: [PATCH 6/8] fix codestyle --- tests/app/service/test_rest.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index cd6bbae8f..316819da0 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3376,8 +3376,8 @@ def test_get_monthly_notification_data_by_service(mocker, admin_request): def test_get_returned_letter_summary(admin_request, sample_service): create_returned_letter(sample_service, reported_at=datetime.utcnow()) - create_returned_letter(sample_service, reported_at=datetime.utcnow()-timedelta(days=3)) + create_returned_letter(sample_service, reported_at=datetime.utcnow() - timedelta(days=3)) response = admin_request.get('service.returned_letter_summary', service_id=sample_service.id) - assert len(response) == 2 \ No newline at end of file + assert len(response) == 2 From 140cb655546e3816eda8a97c68e0fd71f7565034 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 11 Dec 2019 14:41:35 +0000 Subject: [PATCH 7/8] Return report_at date as a date not datetime. I haven't converted this date from UTC to BST because we always upload the returned-letters during business hours. --- app/service/rest.py | 5 ++++- tests/app/dao/test_returned_letters_dao.py | 3 ++- tests/app/db.py | 2 +- tests/app/service/test_rest.py | 6 +++++- 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 4416a5a39..0643d3108 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -12,6 +12,7 @@ from notifications_utils.timezones import convert_utc_to_bst from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound +from app import DATE_FORMAT from app.config import QueueNames from app.dao import fact_notification_status_dao, notifications_dao from app.dao.dao_utils import dao_rollback @@ -946,6 +947,8 @@ def check_if_reply_to_address_already_in_use(service_id, email_address): def returned_letter_summary(service_id): results = get_returned_letter_summary(service_id) - json_results = [{'returned_letter_count': x.returned_letter_count, 'reported_at': x.reported_at} for x in results] + json_results = [{'returned_letter_count': x.returned_letter_count, + 'reported_at': x.reported_at.strftime(DATE_FORMAT) + } for x in results] return jsonify(json_results) diff --git a/tests/app/dao/test_returned_letters_dao.py b/tests/app/dao/test_returned_letters_dao.py index bb0836be0..9c33988a4 100644 --- a/tests/app/dao/test_returned_letters_dao.py +++ b/tests/app/dao/test_returned_letters_dao.py @@ -50,7 +50,7 @@ def test_insert_or_update_returned_letters_updates(sample_letter_template): assert x.notification_id in [notification.id, history.id] -def test_insert_or_update_returned_letters_when_no_notification(sample_letter_template): +def test_insert_or_update_returned_letters_when_no_notification(notify_db_session): insert_or_update_returned_letters(['ref1']) assert ReturnedLetter.query.count() == 0 @@ -104,6 +104,7 @@ def test_get_returned_letter_summary_orders_by_reported_at(sample_service): create_returned_letter(sample_service, reported_at=now) create_returned_letter(sample_service, reported_at=last_month) create_returned_letter(sample_service, reported_at=last_month) + create_returned_letter() # returned letter for a different service results = get_returned_letter_summary(sample_service.id) diff --git a/tests/app/db.py b/tests/app/db.py index ac23654c2..e95f79b77 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -944,7 +944,7 @@ def set_up_usage_data(start_date): return org, org_3, service, service_3, service_4, service_sms_only -def create_returned_letter(service, reported_at=None): +def create_returned_letter(service=None, reported_at=None): if not service: service = create_service(service_name='a - with sms and letter') template = create_template(service=service, template_type=LETTER_TYPE) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 316819da0..ec736abd1 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3374,10 +3374,14 @@ def test_get_monthly_notification_data_by_service(mocker, admin_request): assert response == [] +@freeze_time('2019-12-11 13:30') def test_get_returned_letter_summary(admin_request, sample_service): - create_returned_letter(sample_service, reported_at=datetime.utcnow()) create_returned_letter(sample_service, reported_at=datetime.utcnow() - timedelta(days=3)) + create_returned_letter(sample_service, reported_at=datetime.utcnow()) + create_returned_letter(sample_service, reported_at=datetime.utcnow()) response = admin_request.get('service.returned_letter_summary', service_id=sample_service.id) assert len(response) == 2 + assert response[0] == {'returned_letter_count': 2, 'reported_at': '2019-12-11'} + assert response[1] == {'returned_letter_count': 1, 'reported_at': '2019-12-08'} From d33002544777d1c190437b149a3bafab08d475ac Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 12 Dec 2019 14:11:54 +0000 Subject: [PATCH 8/8] Changed reported_at to a date and included audit columns. --- app/dao/returned_letters_dao.py | 10 ++++++---- app/models.py | 4 +++- .../versions/0310_returned_letters_table_.py | 4 +++- tests/app/dao/test_returned_letters_dao.py | 17 ++++++++++------- tests/app/db.py | 3 ++- 5 files changed, 24 insertions(+), 14 deletions(-) diff --git a/app/dao/returned_letters_dao.py b/app/dao/returned_letters_dao.py index 7e313178c..a79bc5b64 100644 --- a/app/dao/returned_letters_dao.py +++ b/app/dao/returned_letters_dao.py @@ -23,19 +23,21 @@ def _get_notification_ids_for_references(references): @transactional def insert_or_update_returned_letters(references): data = _get_notification_ids_for_references(references) - now = datetime.utcnow() for row in data: table = ReturnedLetter.__table__ stmt = insert(table).values( - reported_at=now, + reported_at=datetime.utcnow().date(), service_id=row.service_id, - notification_id=row.id) + notification_id=row.id, + created_at=datetime.utcnow() + ) stmt = stmt.on_conflict_do_update( index_elements=[table.c.notification_id], set_={ - 'reported_at': now, + 'reported_at': datetime.utcnow().date(), + 'updated_at': datetime.utcnow() } ) db.session.connection().execute(stmt) diff --git a/app/models.py b/app/models.py index 577beb64a..1c7a68f35 100644 --- a/app/models.py +++ b/app/models.py @@ -2076,7 +2076,9 @@ class ReturnedLetter(db.Model): __tablename__ = 'returned_letters' id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - reported_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) + reported_at = db.Column(db.Date, nullable=False) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), unique=False, index=True, nullable=False) service = db.relationship(Service, backref=db.backref('returned_letters')) notification_id = db.Column(UUID(as_uuid=True), unique=True, nullable=False) + created_at = db.Column(db.DateTime, nullable=False) + updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) diff --git a/migrations/versions/0310_returned_letters_table_.py b/migrations/versions/0310_returned_letters_table_.py index a33be2595..fe71e1e7d 100644 --- a/migrations/versions/0310_returned_letters_table_.py +++ b/migrations/versions/0310_returned_letters_table_.py @@ -16,9 +16,11 @@ down_revision = '0309_add_uq_key_row_number' def upgrade(): op.create_table('returned_letters', sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), - sa.Column('reported_at', sa.DateTime(), nullable=False), + sa.Column('reported_at', sa.Date(), nullable=False), sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=False), sa.Column('notification_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=True), sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), sa.PrimaryKeyConstraint('id'), sa.UniqueConstraint('notification_id') diff --git a/tests/app/dao/test_returned_letters_dao.py b/tests/app/dao/test_returned_letters_dao.py index 9c33988a4..126197f1c 100644 --- a/tests/app/dao/test_returned_letters_dao.py +++ b/tests/app/dao/test_returned_letters_dao.py @@ -1,4 +1,4 @@ -from datetime import datetime, timedelta +from datetime import datetime, timedelta, date from freezegun import freeze_time @@ -37,16 +37,19 @@ def test_insert_or_update_returned_letters_updates(sample_letter_template): returned_letters = ReturnedLetter.query.all() assert len(returned_letters) == 2 for x in returned_letters: - assert x.reported_at == datetime(2019, 12, 9, 13, 30) + assert x.reported_at == date(2019, 12, 9) + assert x.created_at == datetime(2019, 12, 9, 13, 30) + assert not x.updated_at assert x.notification_id in [notification.id, history.id] with freeze_time('2019-12-10 14:20'): insert_or_update_returned_letters(['ref1', 'ref2']) - returned_letters = ReturnedLetter.query.all() assert len(returned_letters) == 2 for x in returned_letters: - assert x.reported_at == datetime(2019, 12, 10, 14, 20) + assert x.reported_at == date(2019, 12, 10) + assert x.created_at == datetime(2019, 12, 9, 13, 30) + assert x.updated_at == datetime(2019, 12, 10, 14, 20) assert x.notification_id in [notification.id, history.id] @@ -93,7 +96,7 @@ def test_get_returned_letter_summary(sample_service): assert len(results) == 1 assert results[0].returned_letter_count == 2 - assert results[0].reported_at == now + assert results[0].reported_at == now.date() def test_get_returned_letter_summary_orders_by_reported_at(sample_service): @@ -109,7 +112,7 @@ def test_get_returned_letter_summary_orders_by_reported_at(sample_service): results = get_returned_letter_summary(sample_service.id) assert len(results) == 2 - assert results[0].reported_at == now + assert results[0].reported_at == now.date() assert results[0].returned_letter_count == 3 - assert results[1].reported_at == last_month + assert results[1].reported_at == last_month.date() assert results[1].returned_letter_count == 2 diff --git a/tests/app/db.py b/tests/app/db.py index e95f79b77..e940eab07 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -952,7 +952,8 @@ def create_returned_letter(service=None, reported_at=None): returned_letter = ReturnedLetter( service_id=service.id, reported_at=reported_at or datetime.utcnow(), - notification_id=notification.id + notification_id=notification.id, + created_at=datetime.utcnow(), ) db.session.add(returned_letter)