Merge pull request #2514 from alphagov/dont-insert-notification-history

Look for notification in both tables for complaints

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:
+    notification = dao_get_notification_history_by_reference(notification_update.reference)
-        # 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)
 
     if int(notification_update.page_count) != notification.billable_units:
         msg = 'Notification with id {} has {} billable_units but DVLA says page count is {}'.format(

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(
+    try:
-        NotificationHistory.reference == reference
+        # This try except is necessary because in test keys and research mode does not create notification history.
-    ).one()
+        # 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")

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'])

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
 
 

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'])