Merge branch 'master' of https://github.com/alphagov/notifications-api into secrets-2FA

app/aws/s3.py

@@ -26,3 +26,10 @@ def remove_job_from_s3(service_id, job_id):
     file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id)
     obj = get_s3_object(bucket_name, file_location)
     return obj.delete()
+
+
+def remove_transformed_dvla_file(job_id):
+    bucket_name = current_app.config['DVLA_UPLOAD_BUCKET_NAME']
+    file_location = '{}-dvla-job.text'.format(job_id)
+    obj = get_s3_object(bucket_name, file_location)
+    return obj.delete()

app/celery/scheduled_tasks.py

@@ -24,6 +24,7 @@ from app.dao.provider_details_dao import (
     dao_toggle_sms_provider
 )
 from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago
+from app.models import LETTER_TYPE
 from app.notifications.process_notifications import send_notification_to_queue
 from app.statsd_decorators import statsd
 from app.celery.tasks import process_job
@@ -245,3 +246,12 @@ def delete_inbound_sms_older_than_seven_days():
     except SQLAlchemyError as e:
         current_app.logger.exception("Failed to delete inbound sms notifications")
         raise
+
+
+@notify_celery.task(name="remove_transformed_dvla_files")
+@statsd(namespace="tasks")
+def remove_transformed_dvla_files():
+    jobs = dao_get_jobs_older_than_limited_by(job_types=[LETTER_TYPE])
+    for job in jobs:
+        s3.remove_transformed_dvla_file(job.id)
+        current_app.logger.info("Transformed dvla file for job {} has been removed from s3.".format(job.id))

app/config.py

@@ -204,6 +204,11 @@ class Config(object):
             'options': {'queue': QueueNames.PERIODIC},
             'kwargs': {'job_types': [LETTER_TYPE]}
         },
+        'remove_transformed_dvla_files': {
+            'task': 'remove_transformed_dvla_files',
+            'schedule': crontab(minute=40, hour=4),
+            'options': {'queue': QueueNames.PERIODIC}
+        },
         'timeout-job-statistics': {
             'task': 'timeout-job-statistics',
             'schedule': crontab(minute=0, hour=5),

app/v2/notifications/get_notifications.py

@@ -14,7 +14,7 @@ from app.v2.notifications.notification_schemas import get_notifications_request
 def get_notification_by_id(id):
     try:
         casted_id = uuid.UUID(id)
-    except ValueError or AttributeError:
+    except (ValueError, AttributeError):
         abort(404)
     notification = notifications_dao.get_notification_with_personalisation(
         authenticated_service.id, casted_id, key_type=None

requirements.txt

@@ -13,7 +13,7 @@ flask-marshmallow==0.6.2
 Flask-Bcrypt==0.6.2
 credstash==1.8.0
 boto3==1.4.4
-celery==3.1.23
+celery==3.1.25
 monotonic==1.2
 statsd==3.2.1
 jsonschema==2.5.1

tests/app/aws/test_s3.py

@@ -1,7 +1,11 @@
-from app.aws.s3 import get_s3_file
+from unittest.mock import call
+
+from flask import current_app
+
+from app.aws.s3 import get_s3_file, remove_transformed_dvla_file
 
 
-def test_get_s3_file_makes_correct_call(sample_service, sample_job, mocker):
+def test_get_s3_file_makes_correct_call(notify_api, mocker):
     get_s3_mock = mocker.patch('app.aws.s3.get_s3_object')
     get_s3_file('foo-bucket', 'bar-file.txt')
 
@@ -9,3 +13,15 @@ def test_get_s3_file_makes_correct_call(sample_service, sample_job, mocker):
         'foo-bucket',
         'bar-file.txt'
     )
+
+
+def test_remove_transformed_dvla_file_makes_correct_call(notify_api, mocker):
+    s3_mock = mocker.patch('app.aws.s3.get_s3_object')
+    fake_uuid = '5fbf9799-6b9b-4dbb-9a4e-74a939f3bb49'
+
+    remove_transformed_dvla_file(fake_uuid)
+
+    s3_mock.assert_has_calls([
+        call(current_app.config['DVLA_UPLOAD_BUCKET_NAME'], '{}-dvla-job.text'.format(fake_uuid)),
+        call().delete()
+    ])

tests/app/celery/test_scheduled_tasks.py

@@ -1,10 +1,12 @@
-import pytest
-
 from datetime import datetime, timedelta
 from functools import partial
+from unittest.mock import call, patch, PropertyMock
 
 from flask import current_app
+
+import pytest
 from freezegun import freeze_time
+
 from app.celery import scheduled_tasks
 from app.celery.scheduled_tasks import (
     delete_email_notifications_older_than_seven_days,
@@ -15,6 +17,7 @@ from app.celery.scheduled_tasks import (
     delete_sms_notifications_older_than_seven_days,
     delete_verify_codes,
     remove_csv_files,
+    remove_transformed_dvla_files,
     run_scheduled_jobs,
     s3,
     send_daily_performance_platform_stats,
@@ -41,7 +44,6 @@ from tests.app.conftest import (
     sample_notification_history as create_notification_history,
     create_custom_template)
 from tests.conftest import set_config_values
-from unittest.mock import call, patch, PropertyMock
 
 
 def _create_slow_delivery_notification(provider='mmg'):
@@ -89,6 +91,8 @@ def test_should_have_decorated_tasks_functions():
         'switch_current_sms_provider_on_slow_delivery'
     assert delete_inbound_sms_older_than_seven_days.__wrapped__.__name__ == \
         'delete_inbound_sms_older_than_seven_days'
+    assert remove_transformed_dvla_files.__wrapped__.__name__ == \
+        'remove_transformed_dvla_files'
 
 
 def test_should_call_delete_sms_notifications_more_than_week_in_task(notify_api, mocker):
@@ -488,3 +492,59 @@ def test_remove_csv_files_filters_by_type(mocker, sample_service):
     assert s3.remove_job_from_s3.call_args_list == [
         call(job_to_delete.service_id, job_to_delete.id),
     ]
+
+
+@freeze_time('2017-01-01 10:00:00')
+def test_remove_dvla_transformed_files_removes_expected_files(mocker, sample_service):
+    mocker.patch('app.celery.scheduled_tasks.s3.remove_transformed_dvla_file')
+
+    letter_template = create_template(service=sample_service, template_type=LETTER_TYPE)
+
+    job = partial(create_job, template=letter_template)
+
+    seven_days_ago = datetime.utcnow() - timedelta(days=7)
+    just_under_seven_days = seven_days_ago + timedelta(seconds=1)
+    just_over_seven_days = seven_days_ago - timedelta(seconds=1)
+    eight_days_ago = seven_days_ago - timedelta(days=1)
+    nine_days_ago = eight_days_ago - timedelta(days=1)
+    just_under_nine_days = nine_days_ago + timedelta(seconds=1)
+    just_over_nine_days = nine_days_ago - timedelta(seconds=1)
+
+    job(created_at=seven_days_ago)
+    job(created_at=just_under_seven_days)
+    job_to_delete_1 = job(created_at=just_over_seven_days)
+    job_to_delete_2 = job(created_at=eight_days_ago)
+    job_to_delete_3 = job(created_at=nine_days_ago)
+    job_to_delete_4 = job(created_at=just_under_nine_days)
+    job(created_at=just_over_nine_days)
+
+    remove_transformed_dvla_files()
+
+    s3.remove_transformed_dvla_file.assert_has_calls([
+        call(job_to_delete_1.id),
+        call(job_to_delete_2.id),
+        call(job_to_delete_3.id),
+        call(job_to_delete_4.id),
+    ], any_order=True)
+
+
+def test_remove_dvla_transformed_files_does_not_remove_files(mocker, sample_service):
+    mocker.patch('app.celery.scheduled_tasks.s3.remove_transformed_dvla_file')
+
+    letter_template = create_template(service=sample_service, template_type=LETTER_TYPE)
+
+    job = partial(create_job, template=letter_template)
+
+    yesterday = datetime.utcnow() - timedelta(days=1)
+    six_days_ago = datetime.utcnow() - timedelta(days=6)
+    seven_days_ago = six_days_ago - timedelta(days=1)
+    just_over_nine_days = seven_days_ago - timedelta(days=2, seconds=1)
+
+    job(created_at=yesterday)
+    job(created_at=six_days_ago)
+    job(created_at=seven_days_ago)
+    job(created_at=just_over_nine_days)
+
+    remove_transformed_dvla_files()
+
+    s3.remove_transformed_dvla_file.assert_has_calls([])