From 3374e03ce9aeb0d717c40b75aa234729be512867 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 21 May 2019 16:08:18 +0100 Subject: [PATCH 1/2] Prepare to stop inserting NotificationHistory at the time of inserting a notificaiton. Need to remove foreign key to complaints. Make sure if getting Notification.id we look to both tables. --- app/celery/tasks.py | 9 +------- app/dao/notifications_dao.py | 14 ++++++++++--- migrations/versions/0293_drop_complaint_fk.py | 21 +++++++++++++++++++ tests/app/dao/test_provider_details_dao.py | 2 +- 4 files changed, 34 insertions(+), 12 deletions(-) create mode 100644 migrations/versions/0293_drop_complaint_fk.py diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 7287118a4..a1ee49020 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -18,7 +18,6 @@ from requests import ( RequestException ) from sqlalchemy.exc import SQLAlchemyError -from sqlalchemy.orm.exc import NoResultFound from app import ( create_uuid, @@ -42,7 +41,6 @@ from app.dao.notifications_dao import ( dao_get_last_notification_added_for_job_id, update_notification_status_by_reference, dao_get_notification_history_by_reference, - dao_get_notification_by_reference, ) from app.dao.provider_details_dao import get_current_provider from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id @@ -498,12 +496,7 @@ def update_letter_notification(filename, temporary_failures, update): def check_billable_units(notification_update): - try: - # This try except is necessary because in test keys and research mode does not create notification history. - # Otherwise we could just search for the NotificationHistory object - notification = dao_get_notification_by_reference(notification_update.reference) - except NoResultFound: - notification = dao_get_notification_history_by_reference(notification_update.reference) + notification = dao_get_notification_history_by_reference(notification_update.reference) if int(notification_update.page_count) != notification.billable_units: msg = 'Notification with id {} has {} billable_units but DVLA says page count is {}'.format( diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 35d5759e4..bcd5a5668 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -17,6 +17,7 @@ from notifications_utils.statsd_decorators import statsd from notifications_utils.timezones import convert_utc_to_bst from sqlalchemy import (desc, func, asc) from sqlalchemy.orm import joinedload +from sqlalchemy.orm.exc import NoResultFound from sqlalchemy.sql import functions from sqlalchemy.sql.expression import case from sqlalchemy.dialects.postgresql import insert @@ -585,9 +586,16 @@ def dao_get_notification_by_reference(reference): @statsd(namespace="dao") def dao_get_notification_history_by_reference(reference): - return NotificationHistory.query.filter( - NotificationHistory.reference == reference - ).one() + try: + # This try except is necessary because in test keys and research mode does not create notification history. + # Otherwise we could just search for the NotificationHistory object + return Notification.query.filter( + Notification.reference == reference + ).one() + except NoResultFound: + return NotificationHistory.query.filter( + NotificationHistory.reference == reference + ).one() @statsd(namespace="dao") diff --git a/migrations/versions/0293_drop_complaint_fk.py b/migrations/versions/0293_drop_complaint_fk.py new file mode 100644 index 000000000..84d431f15 --- /dev/null +++ b/migrations/versions/0293_drop_complaint_fk.py @@ -0,0 +1,21 @@ +""" + +Revision ID: 0293_drop_complaint_fk +Revises: 0292_give_users_folder_perms +Create Date: 2019-05-16 14:05:18.104274 + +""" +from alembic import op + + +revision = '0293_drop_complaint_fk' +down_revision = '0292_give_users_folder_perms' + + +def upgrade(): + op.drop_constraint('complaints_notification_id_fkey', table_name='complaints', type_='foreignkey') + + +def downgrade(): + op.create_foreign_key('complaints_notification_id_fkey', 'complaints', + 'notification_history', ['notification_id'], ['id']) diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 31a058831..153f39a2e 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -262,7 +262,7 @@ def test_toggle_sms_provider_switches_provider_stores_notify_user_id_in_history( assert old_provider_from_history.created_by_id == sample_user.id -def test_can_get_all_provider_history(current_sms_provider): +def test_can_get_all_provider_history(restore_provider_details, current_sms_provider): assert len(dao_get_provider_versions(current_sms_provider.id)) == 1 From e731dd96edd517df6255eb2049e4f8fd1e04fa32 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 22 May 2019 10:03:07 +0100 Subject: [PATCH 2/2] Added units test to make sure the complaints work if the notification doesn't exist or if the notification_history doesn't exist --- .../test_notifications_ses_callback.py | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index 80e773768..268c820f0 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -4,8 +4,9 @@ import pytest from flask import json from sqlalchemy.exc import SQLAlchemyError +from app import db from app.dao.notifications_dao import get_notification_by_id -from app.models import Complaint +from app.models import Complaint, NotificationHistory, Notification from app.notifications.notifications_ses_callback import handle_complaint from tests.app.conftest import sample_notification as create_sample_notification @@ -53,6 +54,26 @@ def test_handle_complaint_does_raise_exception_if_notification_not_found(notify_ handle_complaint(response) +def test_process_ses_results_in_complaint_if_notification_history_does_not_exist(sample_email_template): + notification = create_notification(template=sample_email_template, reference='ref1') + NotificationHistory.query.filter_by(id=notification.id).delete() + db.session.commit() + handle_complaint(json.loads(ses_complaint_callback()['Message'])) + complaints = Complaint.query.all() + assert len(complaints) == 1 + assert complaints[0].notification_id == notification.id + + +def test_process_ses_results_in_complaint_if_notification_does_not_exist(sample_email_template): + notification = create_notification(template=sample_email_template, reference='ref1') + Notification.query.filter_by(id=notification.id).delete() + db.session.commit() + handle_complaint(json.loads(ses_complaint_callback()['Message'])) + complaints = Complaint.query.all() + assert len(complaints) == 1 + assert complaints[0].notification_id == notification.id + + def test_process_ses_results_in_complaint_save_complaint_with_null_complaint_type(notify_api, sample_email_template): notification = create_notification(template=sample_email_template, reference='ref1') msg = json.loads(ses_complaint_callback_with_missing_complaint_type()['Message'])