Merge pull request #318 from GSA/badpii

notify-api-317 fix the scrubbing of pii for successful notifications

app/celery/provider_tasks.py

@@ -11,7 +11,7 @@ from app.clients.sms import SmsClientResponseException
 from app.config import QueueNames
 from app.dao import notifications_dao
 from app.dao.notifications_dao import (
-    sanitize_notifications_by_id,
+    sanitize_successful_notification_by_id,
     update_notification_status_by_id,
 )
 from app.delivery import send_to_providers
@@ -37,14 +37,16 @@ def check_sms_delivery_receipt(self, message_id, notification_id, sent_at):
     status, provider_response = aws_cloudwatch_client.check_sms(message_id, notification_id, sent_at)
     if status == 'success':
         status = NOTIFICATION_DELIVERED
-    else:
+    elif status == 'failure':
         status = NOTIFICATION_FAILED
-    update_notification_status_by_id(notification_id, status, provider_response=provider_response)
-    current_app.logger.info(f"Updated notification {notification_id} with response '{provider_response}'")
+    # if status is not success or failure the client raised an exception and this method will retry
 
     if status == NOTIFICATION_DELIVERED:
-        sanitize_notifications_by_id(notification_id)
+        sanitize_successful_notification_by_id(notification_id)
         current_app.logger.info(f"Sanitized notification {notification_id} that was successfully delivered")
+    else:
+        update_notification_status_by_id(notification_id, status, provider_response=provider_response)
+        current_app.logger.info(f"Updated notification {notification_id} with response '{provider_response}'")
 
 
 @notify_celery.task(bind=True, name="deliver_sms", max_retries=48, default_retry_delay=300)

app/clients/cloudwatch/aws_cloudwatch.py

@@ -84,6 +84,6 @@ class AwsCloudwatchClient(Client):
         if all_failed_events and len(all_failed_events) > 0:
             event = all_failed_events[0]
             message = json.loads(event['message'])
-            return "fail", message['delivery']['providerResponse']
+            return "failure", message['delivery']['providerResponse']
 
         raise Exception(f'No event found for message_id {message_id} notification_id {notification_id}')

app/dao/notifications_dao.py

@@ -271,8 +271,7 @@ def _filter_query(query, filter_dict=None):
     return query
 
 
-@autocommit
-def sanitize_notifications_by_id(
+def sanitize_successful_notification_by_id(
     notification_id
 ):
     # TODO what to do for international?
@@ -280,10 +279,9 @@ def sanitize_notifications_by_id(
     Notification.query.filter(
         Notification.id.in_([notification_id]),
     ).update(
-        {'to': phone_prefix, 'normalised_to': phone_prefix},
+        {'to': phone_prefix, 'normalised_to': phone_prefix, 'status': 'delivered'},
         synchronize_session=False
     )
-
     db.session.commit()
 
 

app/notifications/process_notifications.py

@@ -121,8 +121,6 @@ def persist_notification(
         updated_at=updated_at
     )
 
-    current_app.logger.info('Persisting notification with to address: {}'.format(notification.to))
-
     if notification_type == SMS_TYPE:
         formatted_recipient = validate_and_format_phone_number(recipient, international=True)
         recipient_info = get_international_phone_info(formatted_recipient)
@@ -133,7 +131,6 @@ def persist_notification(
     elif notification_type == EMAIL_TYPE:
         current_app.logger.info('Persisting notification with type: {}'.format(EMAIL_TYPE))
         notification.normalised_to = format_email_address(notification.to)
-        current_app.logger.info('Persisting notification to formatted email: {}'.format(notification.normalised_to))
 
     # if simulated create a Notification model to return but do not persist the Notification to the dB
     if not simulated:

tests/app/dao/notification_dao/test_notification_dao.py

@@ -24,6 +24,7 @@ from app.dao.notifications_dao import (
     get_notifications_for_service,
     get_service_ids_with_notifications_on_date,
     notifications_not_yet_sent,
+    sanitize_successful_notification_by_id,
     update_notification_status_by_id,
     update_notification_status_by_reference,
 )
@@ -84,6 +85,26 @@ def test_should_by_able_to_update_status_by_id(sample_template, sample_job, sns_
     assert notification.status == 'delivered'
 
 
+def test_should_be_able_to_sanitize_successful_notification(sample_template, sample_job, sns_provider):
+    with freeze_time('2000-01-01 12:00:00'):
+        data = _notification_json(sample_template, job_id=sample_job.id, status='sending')
+        notification = Notification(**data)
+        notification.to = '15555555555'
+        notification.normalised_to = '15555555555'
+        dao_create_notification(notification)
+        assert notification.status == 'sending'
+        assert notification.normalised_to == '15555555555'
+        assert notification.to == '15555555555'
+
+    assert Notification.query.get(notification.id).status == 'sending'
+
+    with freeze_time('2000-01-02 12:00:00'):
+        sanitize_successful_notification_by_id(notification.id)
+        assert Notification.query.get(notification.id).status == 'delivered'
+        assert Notification.query.get(notification.id).normalised_to == '1'
+        assert Notification.query.get(notification.id).to == '1'
+
+
 def test_should_not_update_status_by_id_if_not_sending_and_does_not_update_job(sample_job):
     notification = create_notification(template=sample_job.template, status='delivered', job=sample_job)
     assert Notification.query.get(notification.id).status == 'delivered'