From 00d2e543dcbbeaf3b5c02bd4d826f69f05cd41b4 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 5 Dec 2017 10:32:19 +0000 Subject: [PATCH 01/25] Refactor letter_rates table to include everything needed to calculate billing for letter notificaitons. It is ok to drop the existing tables as they are not used anywhere as of yet. --- app/models.py | 15 ++---- .../versions/0150_refactor_letter_rates.py | 47 +++++++++++++++++++ 2 files changed, 52 insertions(+), 10 deletions(-) create mode 100644 migrations/versions/0150_refactor_letter_rates.py diff --git a/app/models.py b/app/models.py index 9d81073c7..0a65ae0a7 100644 --- a/app/models.py +++ b/app/models.py @@ -1484,17 +1484,12 @@ class LetterRate(db.Model): __tablename__ = 'letter_rates' id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - valid_from = valid_from = db.Column(db.DateTime, nullable=False) - - -class LetterRateDetail(db.Model): - __tablename__ = 'letter_rate_details' - - id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - letter_rate_id = db.Column(UUID(as_uuid=True), db.ForeignKey('letter_rates.id'), index=True, nullable=False) - letter_rate = db.relationship('LetterRate', backref='letter_rates') - page_total = db.Column(db.Integer, nullable=False) + start_date = db.Column(db.DateTime, nullable=False) + end_date = db.Column(db.DateTime, nullable=True) + sheet_total = db.Column(db.Integer, nullable=False) # double sided sheet rate = db.Column(db.Numeric(), nullable=False) + crown = db.Column(db.Boolean, nullable=False) + post_class = db.Column(db.String, nullable=False) class MonthlyBilling(db.Model): diff --git a/migrations/versions/0150_refactor_letter_rates.py b/migrations/versions/0150_refactor_letter_rates.py new file mode 100644 index 000000000..2429e5222 --- /dev/null +++ b/migrations/versions/0150_refactor_letter_rates.py @@ -0,0 +1,47 @@ +""" + +Revision ID: 0150_refactor_letter_rates +Revises: 0148_add_letters_as_pdf_svc_perm +Create Date: 2017-12-05 10:24:41.232128 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0150_refactor_letter_rates' +down_revision = '0148_add_letters_as_pdf_svc_perm' + + +def upgrade(): + op.drop_table('letter_rate_details') + op.drop_table('letter_rates') + op.create_table('letter_rates', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('start_date', sa.DateTime(), nullable=False), + sa.Column('end_date', sa.DateTime(), nullable=True), + sa.Column('sheet_total', sa.Integer(), nullable=False), + sa.Column('rate', sa.Numeric(), nullable=False), + sa.Column('crown', sa.Boolean(), nullable=False), + sa.Column('post_class', sa.String(), nullable=False), + sa.PrimaryKeyConstraint('id') + ) + + +def downgrade(): + op.drop_table('letter_rates') + op.create_table('letter_rates', + sa.Column('id', postgresql.UUID(), autoincrement=False, nullable=False), + sa.Column('valid_from', postgresql.TIMESTAMP(), autoincrement=False, nullable=False), + sa.PrimaryKeyConstraint('id', name='letter_rates_pkey'), + postgresql_ignore_search_path=False + ) + op.create_table('letter_rate_details', + sa.Column('id', postgresql.UUID(), autoincrement=False, nullable=False), + sa.Column('letter_rate_id', postgresql.UUID(), autoincrement=False, nullable=False), + sa.Column('page_total', sa.INTEGER(), autoincrement=False, nullable=False), + sa.Column('rate', sa.NUMERIC(), autoincrement=False, nullable=False), + sa.ForeignKeyConstraint(['letter_rate_id'], ['letter_rates.id'], + name='letter_rate_details_letter_rate_id_fkey'), + sa.PrimaryKeyConstraint('id', name='letter_rate_details_pkey') + ) From 07a71ef71c7dc69bb64d1f5ffe495b9048bc816f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 5 Dec 2017 13:54:10 +0000 Subject: [PATCH 02/25] Fix db migration script --- migrations/versions/0149_add_crown_to_services.py | 2 +- migrations/versions/0150_refactor_letter_rates.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/migrations/versions/0149_add_crown_to_services.py b/migrations/versions/0149_add_crown_to_services.py index bfbbf976c..0a6412e9d 100644 --- a/migrations/versions/0149_add_crown_to_services.py +++ b/migrations/versions/0149_add_crown_to_services.py @@ -1,6 +1,6 @@ """ -Revision ID: 0149_add_crown_column_to_services +Revision ID: 0149_add_crown_to_services Revises: 0148_add_letters_as_pdf_svc_perm Create Date: 2017-12-04 12:13:35.268712 diff --git a/migrations/versions/0150_refactor_letter_rates.py b/migrations/versions/0150_refactor_letter_rates.py index 2429e5222..b29c7d9a5 100644 --- a/migrations/versions/0150_refactor_letter_rates.py +++ b/migrations/versions/0150_refactor_letter_rates.py @@ -1,7 +1,7 @@ """ Revision ID: 0150_refactor_letter_rates -Revises: 0148_add_letters_as_pdf_svc_perm +Revises: 0149_add_crown_to_services Create Date: 2017-12-05 10:24:41.232128 """ @@ -10,7 +10,7 @@ import sqlalchemy as sa from sqlalchemy.dialects import postgresql revision = '0150_refactor_letter_rates' -down_revision = '0148_add_letters_as_pdf_svc_perm' +down_revision = '0149_add_crown_to_services' def upgrade(): From 14be85160c9348f2913c1231cb053d77b1b315ca Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 6 Dec 2017 16:40:38 +0000 Subject: [PATCH 03/25] Insert initial letter rates. Create letter rates dao. Query to fetch letter rates. --- app/dao/letter_rate_dao.py | 23 +++++++++++ app/models.py | 2 +- .../versions/0150_refactor_letter_rates.py | 26 +++++++++++- tests/app/dao/test_letter_rate_dao.py | 41 +++++++++++++++++++ 4 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 app/dao/letter_rate_dao.py create mode 100644 tests/app/dao/test_letter_rate_dao.py diff --git a/app/dao/letter_rate_dao.py b/app/dao/letter_rate_dao.py new file mode 100644 index 000000000..e69001a44 --- /dev/null +++ b/app/dao/letter_rate_dao.py @@ -0,0 +1,23 @@ +from app import db +from app.dao.dao_utils import transactional +from app.models import LetterRate + + +@transactional +def dao_create_letter_rate(letter_rate): + db.session.add(letter_rate) + + +def get_letter_rates_for_daterange(date, crown, sheet_count, post_class='second'): + rates = LetterRate.query.filter( + LetterRate.start_date <= date + ).filter((LetterRate.end_date == None) | # noqa + (LetterRate.end_date > date) + ).filter( + LetterRate.crown == crown + ).filter( + LetterRate.sheet_count == sheet_count + ).filter( + LetterRate.post_class == post_class + ).all() + return rates diff --git a/app/models.py b/app/models.py index d5c4ea5af..e36e91491 100644 --- a/app/models.py +++ b/app/models.py @@ -1487,7 +1487,7 @@ class LetterRate(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) start_date = db.Column(db.DateTime, nullable=False) end_date = db.Column(db.DateTime, nullable=True) - sheet_total = db.Column(db.Integer, nullable=False) # double sided sheet + sheet_count = db.Column(db.Integer, nullable=False) # double sided sheet rate = db.Column(db.Numeric(), nullable=False) crown = db.Column(db.Boolean, nullable=False) post_class = db.Column(db.String, nullable=False) diff --git a/migrations/versions/0150_refactor_letter_rates.py b/migrations/versions/0150_refactor_letter_rates.py index b29c7d9a5..1b2555438 100644 --- a/migrations/versions/0150_refactor_letter_rates.py +++ b/migrations/versions/0150_refactor_letter_rates.py @@ -5,6 +5,9 @@ Revises: 0149_add_crown_to_services Create Date: 2017-12-05 10:24:41.232128 """ +import uuid +from datetime import datetime + from alembic import op import sqlalchemy as sa from sqlalchemy.dialects import postgresql @@ -20,13 +23,34 @@ def upgrade(): sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), sa.Column('start_date', sa.DateTime(), nullable=False), sa.Column('end_date', sa.DateTime(), nullable=True), - sa.Column('sheet_total', sa.Integer(), nullable=False), + sa.Column('sheet_count', sa.Integer(), nullable=False), sa.Column('rate', sa.Numeric(), nullable=False), sa.Column('crown', sa.Boolean(), nullable=False), sa.Column('post_class', sa.String(), nullable=False), sa.PrimaryKeyConstraint('id') ) + start_date = datetime(2016, 3, 31, 23, 00, 00) + op.execute("insert into letter_rates values('{}', '{}', null, 1, 0.30, True, 'second')".format( + str(uuid.uuid4()), start_date) + ) + op.execute("insert into letter_rates values('{}', '{}', null, 2, 0.33, True, 'second')".format( + str(uuid.uuid4()), start_date) + ) + op.execute("insert into letter_rates values('{}', '{}', null, 3, 0.36, True, 'second')".format( + str(uuid.uuid4()), start_date) + ) + + op.execute("insert into letter_rates values('{}', '{}', null, 1, 0.33, False, 'second')".format( + str(uuid.uuid4()), start_date) + ) + op.execute("insert into letter_rates values('{}', '{}', null, 2, 0.39, False, 'second')".format( + str(uuid.uuid4()), start_date) + ) + op.execute("insert into letter_rates values('{}', '{}', null, 3, 0.45, False, 'second')".format( + str(uuid.uuid4()), start_date) + ) + def downgrade(): op.drop_table('letter_rates') diff --git a/tests/app/dao/test_letter_rate_dao.py b/tests/app/dao/test_letter_rate_dao.py new file mode 100644 index 000000000..f3fd0d139 --- /dev/null +++ b/tests/app/dao/test_letter_rate_dao.py @@ -0,0 +1,41 @@ +from datetime import datetime + +from decimal import Decimal + +from app.dao.letter_rate_dao import dao_create_letter_rate, get_letter_rates_for_daterange +from app.models import LetterRate + + +def test_dao_create_letter_rate(notify_db_session): + letter_rate = LetterRate(start_date=datetime(2017, 12, 1), + rate=0.33, + crown=True, + sheet_count=1, + post_class='second') + + dao_create_letter_rate(letter_rate) + + rates = LetterRate.query.all() + assert len(rates) == 1 + + +def test_get_letter_rates_for_daterange(notify_db_session): + letter_rate = LetterRate(start_date=datetime(2017, 12, 1), + rate=0.33, + crown=True, + sheet_count=1, + post_class='second') + + dao_create_letter_rate(letter_rate) + letter_rate = LetterRate(start_date=datetime(2016, 12, 1), + end_date=datetime(2017, 12, 1), + rate=0.30, + crown=True, + sheet_count=1, + post_class='second') + + dao_create_letter_rate(letter_rate) + + results = get_letter_rates_for_daterange(date=datetime(2017, 12, 2), crown=True, sheet_count=1, post_class='second') + assert len(results) == 1 + assert results[0].rate == Decimal('0.33') From da656af6e023cd3844ddd896e9937c5845f327a3 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 6 Dec 2017 17:26:32 +0000 Subject: [PATCH 04/25] Fix merge conflict with db script --- ...ctor_letter_rates.py => 0151_refactor_letter_rates.py} | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) rename migrations/versions/{0150_refactor_letter_rates.py => 0151_refactor_letter_rates.py} (94%) diff --git a/migrations/versions/0150_refactor_letter_rates.py b/migrations/versions/0151_refactor_letter_rates.py similarity index 94% rename from migrations/versions/0150_refactor_letter_rates.py rename to migrations/versions/0151_refactor_letter_rates.py index 1b2555438..7a969cc42 100644 --- a/migrations/versions/0150_refactor_letter_rates.py +++ b/migrations/versions/0151_refactor_letter_rates.py @@ -1,7 +1,7 @@ """ -Revision ID: 0150_refactor_letter_rates -Revises: 0149_add_crown_to_services +Revision ID: 0151_refactor_letter_rates +Revises: 0150_another_letter_org Create Date: 2017-12-05 10:24:41.232128 """ @@ -12,8 +12,8 @@ from alembic import op import sqlalchemy as sa from sqlalchemy.dialects import postgresql -revision = '0150_refactor_letter_rates' -down_revision = '0149_add_crown_to_services' +revision = '0151_refactor_letter_rates' +down_revision = '0150_another_letter_org' def upgrade(): From 68659bab1b0b2a0190f3b1fe003c31feb6627572 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 7 Dec 2017 11:58:49 +0000 Subject: [PATCH 05/25] Fixed processing of incomplete letter jobs - `template` argument passed in to `job_complete` should be `template.template_type` otherwise the job status is incorrectly set --- app/celery/tasks.py | 2 +- tests/app/celery/test_tasks.py | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 53d714213..54d6090db 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -580,4 +580,4 @@ def process_incomplete_job(job_id): if row_number > resume_from_row: process_row(row_number, recipient, personalisation, template, job, job.service) - job_complete(job, job.service, template, resumed=True) + job_complete(job, job.service, template.template_type, resumed=True) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index e33956a56..aceb525a0 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1439,6 +1439,7 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_letter')) mock_letter_saver = mocker.patch('app.celery.tasks.save_letter.apply_async') + mock_build_dvla = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') job = create_job(template=sample_letter_template, notification_count=10, created_at=datetime.utcnow() - timedelta(hours=2), @@ -1453,8 +1454,5 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template): process_incomplete_job(str(job.id)) - completed_job = Job.query.filter(Job.id == job.id).one() - - assert completed_job.job_status == JOB_STATUS_FINISHED - + assert mock_build_dvla.called assert mock_letter_saver.call_count == 8 From 7e5005b4dfa06353d840d1822366e9cd4e3c036a Mon Sep 17 00:00:00 2001 From: kentsanggds Date: Thu, 7 Dec 2017 14:47:00 +0000 Subject: [PATCH 06/25] Update requirements to fix html5lib issue --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 2ae3d9a06..083649188 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,7 +26,7 @@ notifications-python-client==4.6.0 awscli==1.14.4 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@23.2.1#egg=notifications-utils==23.2.1 +git+https://github.com/alphagov/notifications-utils.git@23.2.1#egg=notifications-utils==23.3.1 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From 21f6e78495d39bbd3bf1b0f56ab74f1f3d404e20 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 7 Dec 2017 15:07:54 +0000 Subject: [PATCH 07/25] Fix incomplete version number specification It needs setting to the new one in both places. --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 083649188..0c0b2432e 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,7 +26,7 @@ notifications-python-client==4.6.0 awscli==1.14.4 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@23.2.1#egg=notifications-utils==23.3.1 +git+https://github.com/alphagov/notifications-utils.git@23.3.1#egg=notifications-utils==23.3.1 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From 46226a25b41711995a45e1888759efb604e6fbae Mon Sep 17 00:00:00 2001 From: venusbb Date: Thu, 7 Dec 2017 16:37:36 +0000 Subject: [PATCH 08/25] modified dao_timeout_notifications added send delivery status in scheduled timeout sending email/sms tasks --- app/celery/scheduled_tasks.py | 16 +++++++++++++--- app/dao/notifications_dao.py | 17 ++++++++++++----- .../notification_dao/test_notification_dao.py | 12 ++++++------ 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 697fd2eef..ed14be2e4 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -58,6 +58,8 @@ from app.celery.tasks import ( from app.config import QueueNames, TaskNames from app.utils import convert_utc_to_bst from app.v2.errors import JobIncompleteError +from app.dao.service_callback_api_dao import get_service_callback_api_for_service +from app.celery.service_callback_tasks import send_delivery_status_to_service @worker_process_shutdown.connect @@ -189,10 +191,18 @@ def delete_invitations(): @notify_celery.task(name='timeout-sending-notifications') @statsd(namespace="tasks") def timeout_notifications(): - updated = dao_timeout_notifications(current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD')) - if updated: + notifications = dao_timeout_notifications(current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD')) + + if notifications: + for notification in notifications: + # queue callback task only if the service_callback_api exists + service_callback_api = get_service_callback_api_for_service(service_id=notification.service_id) + + if service_callback_api: + send_delivery_status_to_service.apply_async([str(id)], queue=QueueNames.NOTIFY) + current_app.logger.info( - "Timeout period reached for {} notifications, status has been updated.".format(updated)) + "Timeout period reached for {} notifications, status has been updated.".format(len(notifications))) @notify_celery.task(name='send-daily-performance-platform-stats') diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 1f7b33167..d9886fcfd 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -328,17 +328,24 @@ def dao_delete_notifications_and_history_by_id(notification_id): def _timeout_notifications(current_statuses, new_status, timeout_start, updated_at): + + notifications = Notification.query.filter( + Notification.created_at < timeout_start, + Notification.status.in_(current_statuses), + Notification.notification_type != LETTER_TYPE + ).all() for table in [NotificationHistory, Notification]: q = table.query.filter( table.created_at < timeout_start, table.status.in_(current_statuses), table.notification_type != LETTER_TYPE ) - last_update_count = q.update( + q.update( {'status': new_status, 'updated_at': updated_at}, synchronize_session=False ) - return last_update_count + # return a list of q = notification_ids in Notification table for sending delivery receipts + return notifications def dao_timeout_notifications(timeout_period_in_seconds): @@ -359,14 +366,14 @@ def dao_timeout_notifications(timeout_period_in_seconds): timeout = functools.partial(_timeout_notifications, timeout_start=timeout_start, updated_at=updated_at) # Notifications still in created status are marked with a technical-failure: - updated = timeout([NOTIFICATION_CREATED], NOTIFICATION_TECHNICAL_FAILURE) + updated_ids = timeout([NOTIFICATION_CREATED], NOTIFICATION_TECHNICAL_FAILURE) # Notifications still in sending or pending status are marked with a temporary-failure: - updated += timeout([NOTIFICATION_SENDING, NOTIFICATION_PENDING], NOTIFICATION_TEMPORARY_FAILURE) + updated_ids += timeout([NOTIFICATION_SENDING, NOTIFICATION_PENDING], NOTIFICATION_TEMPORARY_FAILURE) db.session.commit() - return updated + return updated_ids def get_total_sent_notifications_in_date_range(start_date, end_date, notification_type): diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 0bb9b5322..8af596199 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -1201,7 +1201,7 @@ def test_dao_timeout_notifications(sample_template): assert Notification.query.get(sending.id).status == 'sending' assert Notification.query.get(pending.id).status == 'pending' assert Notification.query.get(delivered.id).status == 'delivered' - updated = dao_timeout_notifications(1) + updated_ids = dao_timeout_notifications(1) assert Notification.query.get(created.id).status == 'technical-failure' assert Notification.query.get(sending.id).status == 'temporary-failure' assert Notification.query.get(pending.id).status == 'temporary-failure' @@ -1210,7 +1210,7 @@ def test_dao_timeout_notifications(sample_template): assert NotificationHistory.query.get(sending.id).status == 'temporary-failure' assert NotificationHistory.query.get(pending.id).status == 'temporary-failure' assert NotificationHistory.query.get(delivered.id).status == 'delivered' - assert updated == 3 + assert len(updated_ids) == 3 def test_dao_timeout_notifications_only_updates_for_older_notifications(sample_template): @@ -1224,12 +1224,12 @@ def test_dao_timeout_notifications_only_updates_for_older_notifications(sample_t assert Notification.query.get(sending.id).status == 'sending' assert Notification.query.get(pending.id).status == 'pending' assert Notification.query.get(delivered.id).status == 'delivered' - updated = dao_timeout_notifications(1) + updated_ids = dao_timeout_notifications(1) assert NotificationHistory.query.get(created.id).status == 'created' assert NotificationHistory.query.get(sending.id).status == 'sending' assert NotificationHistory.query.get(pending.id).status == 'pending' assert NotificationHistory.query.get(delivered.id).status == 'delivered' - assert updated == 0 + assert len(updated_ids) == 0 def test_dao_timeout_notifications_doesnt_affect_letters(sample_letter_template): @@ -1244,13 +1244,13 @@ def test_dao_timeout_notifications_doesnt_affect_letters(sample_letter_template) assert Notification.query.get(pending.id).status == 'pending' assert Notification.query.get(delivered.id).status == 'delivered' - updated = dao_timeout_notifications(1) + updated_ids = dao_timeout_notifications(1) assert NotificationHistory.query.get(created.id).status == 'created' assert NotificationHistory.query.get(sending.id).status == 'sending' assert NotificationHistory.query.get(pending.id).status == 'pending' assert NotificationHistory.query.get(delivered.id).status == 'delivered' - assert updated == 0 + assert len(updated_ids) == 0 def test_should_return_notifications_excluding_jobs_by_default(sample_template, sample_job, sample_api_key): From 624acf9ffe734426d237c00044558ae4156f3eac Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 8 Dec 2017 15:01:19 +0000 Subject: [PATCH 09/25] Removed the events data for the service/{id}/history page. It does not make sense there. --- app/service/rest.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index d71cd2b90..9c1763af0 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -286,12 +286,11 @@ def get_service_provider_aggregate_statistics(service_id): # tables. This is so product owner can pass stories as done @service_blueprint.route('//history', methods=['GET']) def get_service_history(service_id): - from app.models import (Service, ApiKey, TemplateHistory, Event) + from app.models import (Service, ApiKey, TemplateHistory) from app.schemas import ( service_history_schema, api_key_history_schema, - template_history_schema, - event_schema + template_history_schema ) service_history = Service.get_history_model().query.filter_by(id=service_id).all() @@ -302,14 +301,11 @@ def get_service_history(service_id): template_history = TemplateHistory.query.filter_by(service_id=service_id).all() template_data, errors = template_history_schema.dump(template_history, many=True) - events = Event.query.all() - events_data = event_schema.dump(events, many=True).data - data = { 'service_history': service_data, 'api_key_history': api_keys_data, 'template_history': template_data, - 'events': events_data} + 'events': []} return jsonify(data=data) From a26588decd653ea729c82b94abfd07073c5877db Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 11 Dec 2017 17:14:36 +0000 Subject: [PATCH 10/25] Update the json in the service callback task to read completed_at rather than updated_at --- app/celery/service_callback_tasks.py | 2 +- tests/app/celery/test_service_callback_tasks.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/celery/service_callback_tasks.py b/app/celery/service_callback_tasks.py index 3bf033a80..19f322842 100644 --- a/app/celery/service_callback_tasks.py +++ b/app/celery/service_callback_tasks.py @@ -34,7 +34,7 @@ def send_delivery_status_to_service(self, notification_id): "to": notification.to, "status": notification.status, "created_at": notification.created_at.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request - "updated_at": notification.updated_at.strftime(DATETIME_FORMAT), # the last time the status was updated + "completed_at": notification.updated_at.strftime(DATETIME_FORMAT), # the last time the status was updated "sent_at": notification.sent_at.strftime(DATETIME_FORMAT), # the time the email was sent "notification_type": notification.notification_type } diff --git a/tests/app/celery/test_service_callback_tasks.py b/tests/app/celery/test_service_callback_tasks.py index f09333d2d..a7c9aaeb4 100644 --- a/tests/app/celery/test_service_callback_tasks.py +++ b/tests/app/celery/test_service_callback_tasks.py @@ -55,7 +55,7 @@ def test_send_delivery_status_to_service_post_https_request_to_service(notify_db "to": notification.to, "status": notification.status, "created_at": datestr.strftime(DATETIME_FORMAT), # the time GOV.UK email sent the request - "updated_at": datestr.strftime(DATETIME_FORMAT), # the last time the status was updated + "completed_at": datestr.strftime(DATETIME_FORMAT), # the last time the status was updated "sent_at": datestr.strftime(DATETIME_FORMAT), # the time the email was sent "notification_type": notification_type } From c8a434fe98831ce1abef6f50aaa32425fe9b5394 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 6 Dec 2017 13:51:52 +0000 Subject: [PATCH 11/25] Updated config for template preview env vars --- app/config.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/config.py b/app/config.py index d553041e3..27ba485fb 100644 --- a/app/config.py +++ b/app/config.py @@ -304,6 +304,9 @@ class Config(object): # {"dataset_1": "token_1", ...} PERFORMANCE_PLATFORM_ENDPOINTS = json.loads(os.environ.get('PERFORMANCE_PLATFORM_ENDPOINTS', '{}')) + TEMPLATE_PREVIEW_API_HOST = os.environ.get('TEMPLATE_PREVIEW_API_HOST', 'http://localhost:6013') + TEMPLATE_PREVIEW_API_KEY = os.environ.get('TEMPLATE_PREVIEW_API_KEY', 'my-secret-key') + ###################### # Config overrides ### From 16136317f9a2e826a5f352c02939ccc57efbc73a Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 6 Dec 2017 13:54:02 +0000 Subject: [PATCH 12/25] Add letters pdf task --- app/celery/tasks.py | 78 ++++++++++++++++++++++++++++++++-- tests/app/celery/test_tasks.py | 2 +- tests/app/conftest.py | 4 ++ 3 files changed, 80 insertions(+), 4 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 54d6090db..08cd4a92e 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -4,6 +4,8 @@ from collections import namedtuple from celery.signals import worker_process_shutdown from flask import current_app +import requests + from notifications_utils.recipients import ( RecipientCSV ) @@ -18,6 +20,7 @@ from requests import ( RequestException ) from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm.exc import NoResultFound from botocore.exceptions import ClientError as BotoClientError from app import ( @@ -44,7 +47,8 @@ from app.dao.notifications_dao import ( dao_update_notifications_for_job_to_sent_to_dvla, dao_update_notifications_by_reference, dao_get_last_notification_added_for_job_id, - dao_get_notifications_by_references + dao_get_notifications_by_references, + update_notification_status_by_id ) from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service @@ -121,11 +125,17 @@ def process_job(job_id): def job_complete(job, service, template_type, resumed=False, start=None): - if template_type == LETTER_TYPE: + if ( + template_type == LETTER_TYPE and + 'letters_as_pdf' not in [p.permission for p in service.permissions] + ): if service.research_mode: update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) else: - build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) + build_dvla_file.apply_async( + [str(job.id)], + queue=QueueNames.JOBS + ) current_app.logger.info("send job {} to build-dvla-file in the {} queue".format(job.id, QueueNames.JOBS)) else: job.job_status = JOB_STATUS_FINISHED @@ -312,6 +322,14 @@ def save_letter( reply_to_text=service.get_default_letter_contact() ) + if ( + 'letters_as_pdf' in [p.permission for p in service.permissions] and not service.research_mode + ): + create_letters_pdf.apply_async( + [str(saved_notification.id)], + queue=QueueNames.JOBS + ) + current_app.logger.info("Letter {} created at {}".format(saved_notification.id, saved_notification.created_at)) except SQLAlchemyError as e: handle_exception(self, notification, notification_id, e) @@ -581,3 +599,57 @@ def process_incomplete_job(job_id): process_row(row_number, recipient, personalisation, template, job, job.service) job_complete(job, job.service, template.template_type, resumed=True) + + +@notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300) +@statsd(namespace="tasks") +def create_letters_pdf(self, notification_id): + try: + notification = get_notification_by_id(notification_id) + if not notification: + raise NoResultFound() + + pdf_data = get_letters_pdf( + notification.template, + contact_block=notification.reply_to_text, + org_id=notification.service.dvla_organisation.id, + values=notification.personalisation + ) + current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format( + notification.id, notification.reference, notification.created_at, len(pdf_data))) + s3.upload_letters_pdf(reference=notification.reference, crown=True, filedata=pdf_data) + except (RequestException, BotoClientError): + try: + current_app.logger.exception( + "Letters PDF notification creation for id: {} failed".format(notification_id) + ) + self.retry(queue=QueueNames.RETRY) + except self.MaxRetriesExceededError: + current_app.logger.exception( + "RETRY FAILED: task create_letters_pdf failed for notification {}".format(notification_id), + ) + update_notification_status_by_id(notification_id, 'technical-failure') + + +def get_letters_pdf(template, contact_block=None, org_id='001', values=None): + template_for_letter_print = { + "subject": template.subject, + "content": template.content + } + + data = { + 'letter_contact_block': contact_block, + 'template': template_for_letter_print, + 'values': values, + 'dvla_org_id': org_id, + } + resp = requests.post( + '{}/print.pdf'.format( + current_app.config['TEMPLATE_PREVIEW_API_HOST'] + ), + json=data, + headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} + ) + resp.raise_for_status() + + return resp.content diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index aceb525a0..f76eac84c 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1436,7 +1436,6 @@ def test_process_incomplete_job_email(mocker, sample_email_template): def test_process_incomplete_job_letter(mocker, sample_letter_template): - mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_letter')) mock_letter_saver = mocker.patch('app.celery.tasks.save_letter.apply_async') mock_build_dvla = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') @@ -1456,3 +1455,4 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template): assert mock_build_dvla.called assert mock_letter_saver.call_count == 8 + \ No newline at end of file diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 71b510767..3706494f2 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -299,6 +299,10 @@ def sample_template_without_email_permission(notify_db, notify_db_session): @pytest.fixture def sample_letter_template(sample_service_full_permissions): + # remove letters_as_pdf from fixture until we drop building of dvla files + from app.dao.service_permissions_dao import dao_remove_service_permission + dao_remove_service_permission(sample_service_full_permissions.id, 'letters_as_pdf') + return create_template(sample_service_full_permissions, template_type=LETTER_TYPE) From bad129c5e9c6e4c4342d1c2a7eb3c2613bea9a7f Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 7 Dec 2017 17:04:23 +0000 Subject: [PATCH 13/25] Add extra logging for upload_letters_pdf --- app/aws/s3.py | 3 +++ tests/app/celery/test_tasks.py | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index a64be797c..4551c8c72 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -99,3 +99,6 @@ def upload_letters_pdf(reference, crown, filedata): bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], file_location=upload_file_name ) + + current_app.logger.info("Uploading letters PDF {} to {}".format( + upload_file_name, current_app.config['LETTERS_PDF_BUCKET_NAME'])) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index f76eac84c..481cfd41b 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1455,4 +1455,3 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template): assert mock_build_dvla.called assert mock_letter_saver.call_count == 8 - \ No newline at end of file From 6b118ec1ef25a4a69f6823cf5ef523eaec20a8db Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 8 Dec 2017 17:27:05 +0000 Subject: [PATCH 14/25] Add create-letters-pdf queue name --- app/config.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/config.py b/app/config.py index 27ba485fb..4ec4c3f75 100644 --- a/app/config.py +++ b/app/config.py @@ -30,6 +30,7 @@ class QueueNames(object): RETRY = 'retry-tasks' NOTIFY = 'notify-internal-tasks' PROCESS_FTP = 'process-ftp-tasks' + CREATE_LETTERS_PDF = 'create-letters-pdf' @staticmethod def all_queues(): @@ -44,6 +45,7 @@ class QueueNames(object): QueueNames.JOBS, QueueNames.RETRY, QueueNames.NOTIFY, + QueueNames.CREATE_LETTERS_PDF, ] From 9784ee438a3ab518faaceeeff2c80dec2cda501d Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 8 Dec 2017 17:32:18 +0000 Subject: [PATCH 15/25] Refactored code in tasks.py --- app/celery/tasks.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 08cd4a92e..9b4d2e398 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -132,10 +132,7 @@ def job_complete(job, service, template_type, resumed=False, start=None): if service.research_mode: update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) else: - build_dvla_file.apply_async( - [str(job.id)], - queue=QueueNames.JOBS - ) + build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) current_app.logger.info("send job {} to build-dvla-file in the {} queue".format(job.id, QueueNames.JOBS)) else: job.job_status = JOB_STATUS_FINISHED @@ -146,7 +143,7 @@ def job_complete(job, service, template_type, resumed=False, start=None): if resumed: current_app.logger.info( - "Resumed Job {} completed at {}".format(job.id, job.created_at, start, finished) + "Resumed Job {} completed at {}".format(job.id, job.created_at) ) else: current_app.logger.info( @@ -327,7 +324,7 @@ def save_letter( ): create_letters_pdf.apply_async( [str(saved_notification.id)], - queue=QueueNames.JOBS + queue=QueueNames.CREATE_LETTERS_PDF ) current_app.logger.info("Letter {} created at {}".format(saved_notification.id, saved_notification.created_at)) @@ -631,7 +628,7 @@ def create_letters_pdf(self, notification_id): update_notification_status_by_id(notification_id, 'technical-failure') -def get_letters_pdf(template, contact_block=None, org_id='001', values=None): +def get_letters_pdf(template, contact_block, org_id, values=None): template_for_letter_print = { "subject": template.subject, "content": template.content From bc316dd1fe8ca384d70160d661b5cc3c3843ef5b Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 8 Dec 2017 17:32:57 +0000 Subject: [PATCH 16/25] Add reference as part of sample_letter_notification --- tests/app/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 3706494f2..c8f06e8c4 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -603,7 +603,7 @@ def sample_letter_notification(sample_letter_template): 'address_line_6': 'A6', 'postcode': 'A_POST' } - return create_notification(sample_letter_template, personalisation=address) + return create_notification(sample_letter_template, reference='foo', personalisation=address) @pytest.fixture(scope='function') From 7970cd3d4e97bd45f260b62a7d7215c007a5ecb7 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 8 Dec 2017 17:33:13 +0000 Subject: [PATCH 17/25] Update test for queue names --- tests/app/test_config.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/app/test_config.py b/tests/app/test_config.py index 3a40b4db4..c023fa37a 100644 --- a/tests/app/test_config.py +++ b/tests/app/test_config.py @@ -63,7 +63,7 @@ def test_cloudfoundry_config_has_different_defaults(): def test_queue_names_all_queues_correct(): # Need to ensure that all_queues() only returns queue names used in API queues = QueueNames.all_queues() - assert len(queues) == 10 + assert len(queues) == 11 assert set([ QueueNames.PRIORITY, QueueNames.PERIODIC, @@ -74,5 +74,6 @@ def test_queue_names_all_queues_correct(): QueueNames.STATISTICS, QueueNames.JOBS, QueueNames.RETRY, - QueueNames.NOTIFY + QueueNames.NOTIFY, + QueueNames.CREATE_LETTERS_PDF ]) == set(queues) From aca52952d8db95a25a9101e0229302bc06600528 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 8 Dec 2017 17:40:00 +0000 Subject: [PATCH 18/25] Add tests for letters as pdf permission - test that `create_letters_pdf` only gets called when `letters_as_pdf` permission set - test that `build_dvla_file` does not get called when `letters_as_pdf` permission set - test creating the pdf call to notifications template preview service - test uploading the data to s3 calls upload_letters_pdf function --- tests/app/celery/test_tasks.py | 134 ++++++++++++++++++++++++++++++++- 1 file changed, 132 insertions(+), 2 deletions(-) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 481cfd41b..7af5c0a06 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -9,8 +9,9 @@ from flask import current_app from freezegun import freeze_time from requests import RequestException from sqlalchemy.exc import SQLAlchemyError -from celery.exceptions import Retry +from celery.exceptions import Retry, MaxRetriesExceededError from botocore.exceptions import ClientError +from sqlalchemy.orm.exc import NoResultFound from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate, LetterDVLATemplate from app import (encryption, DATETIME_FORMAT) @@ -20,6 +21,9 @@ from app.celery.scheduled_tasks import check_job_status from app.celery.tasks import ( build_dvla_file, create_dvla_file_contents_for_job, + create_letters_pdf, + get_letters_pdf, + job_complete, process_job, process_row, save_sms, @@ -32,7 +36,7 @@ from app.celery.tasks import ( send_inbound_sms_to_service, ) from app.config import QueueNames -from app.dao import jobs_dao, services_dao +from app.dao import jobs_dao, services_dao, service_permissions_dao from app.models import ( Job, Notification, @@ -47,6 +51,7 @@ from app.models import ( SMS_TYPE ) +from tests.conftest import set_config_values from tests.app import load_example_csv from tests.app.conftest import ( sample_service as create_sample_service, @@ -94,6 +99,7 @@ def test_should_have_decorated_tasks_functions(): assert save_sms.__wrapped__.__name__ == 'save_sms' assert save_email.__wrapped__.__name__ == 'save_email' assert save_letter.__wrapped__.__name__ == 'save_letter' + assert create_letters_pdf.__wrapped__.__name__ == 'create_letters_pdf' @pytest.fixture @@ -1039,6 +1045,50 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session): assert notification_db.reply_to_text == "Address contact" +def test_save_letter_saves_letter_calls_create_letters_as_pdf_with_letters_as_pdf_permission( + mocker, notify_db_session, sample_letter_job): + service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf') + mock_create_letters_pdf = mocker.patch('app.celery.tasks.create_letters_pdf.apply_async') + + personalisation = { + 'addressline1': 'Foo', + 'addressline2': 'Bar', + 'postcode': 'Flob', + } + notification_json = _notification_json( + template=sample_letter_job.template, + to='Foo', + personalisation=personalisation, + job_id=sample_letter_job.id, + row_number=1 + ) + notification_id = uuid.uuid4() + + save_letter( + sample_letter_job.service_id, + notification_id, + encryption.encrypt(notification_json), + ) + + assert mock_create_letters_pdf.called + mock_create_letters_pdf.assert_called_once_with( + [str(notification_id)], + queue=QueueNames.CREATE_LETTERS_PDF + ) + + +def test_job_complete_does_not_call_build_dvla_file_with_letters_as_pdf_permission( + mocker, notify_db_session, sample_letter_job): + service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf') + mock_build_dvla_files = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') + + job_complete(sample_letter_job, sample_letter_job.service, sample_letter_job.template.template_type) + + assert not sample_letter_job.service.research_mode + assert not mock_build_dvla_files.called + assert sample_letter_job.job_status == JOB_STATUS_FINISHED + + def test_should_cancel_job_if_service_is_inactive(sample_service, sample_job, mocker): @@ -1455,3 +1505,83 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template): assert mock_build_dvla.called assert mock_letter_saver.call_count == 8 + + +@pytest.mark.parametrize('personalisation', [{'name': 'test'}, None]) +def test_get_letters_pdf_calls_notifications_template_preview_service_correctly( + notify_api, mocker, client, sample_letter_template, personalisation): + contact_block = 'Mr Foo,\n1 Test Street,\nLondon\nN1' + dvla_org_id = '002' + + with set_config_values(notify_api, { + 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', + 'TEMPLATE_PREVIEW_API_KEY': 'test-key' + }): + with requests_mock.Mocker() as request_mock: + mock_post = request_mock.post( + 'http://localhost/notifications-template-preview/print.pdf', content=b'\x00\x01', status_code=200) + + get_letters_pdf( + sample_letter_template, contact_block=contact_block, org_id=dvla_org_id, values=personalisation) + + assert mock_post.last_request.json() == { + 'values': personalisation, + 'letter_contact_block': contact_block, + 'dvla_org_id': dvla_org_id, + 'template': { + 'subject': sample_letter_template.subject, + 'content': sample_letter_template.content + } + } + + +def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notification): + mocker.patch('app.celery.tasks.get_letters_pdf', return_value=b'\x00\x01') + mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf') + + create_letters_pdf(sample_letter_notification.id) + + mock_s3.assert_called_with( + reference=sample_letter_notification.reference, + crown=True, + filedata=b'\x00\x01' + ) + + +def test_create_letters_pdf_non_existent_notification(notify_api, mocker, fake_uuid): + with pytest.raises(expected_exception=NoResultFound): + create_letters_pdf(fake_uuid) + + +def test_create_letters_pdf_handles_request_errors(mocker, sample_letter_notification): + mock_get_letters_pdf = mocker.patch('app.celery.tasks.get_letters_pdf', side_effect=RequestException) + mock_retry = mocker.patch('app.celery.tasks.create_letters_pdf.retry') + + create_letters_pdf(sample_letter_notification.id) + + assert mock_get_letters_pdf.called + assert mock_retry.called + + +def test_create_letters_pdf_handles_s3_errors(mocker, sample_letter_notification): + mocker.patch('app.celery.tasks.get_letters_pdf') + mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf', side_effect=ClientError({}, 'operation_name')) + mock_retry = mocker.patch('app.celery.tasks.create_letters_pdf.retry') + + create_letters_pdf(sample_letter_notification.id) + + assert mock_s3.called + assert mock_retry.called + + +def test_create_letters_pdf_sets_technical_failure_max_retries(mocker, sample_letter_notification): + mock_get_letters_pdf = mocker.patch('app.celery.tasks.get_letters_pdf', side_effect=RequestException) + mock_retry = mocker.patch('app.celery.tasks.create_letters_pdf.retry', side_effect=MaxRetriesExceededError) + mock_update_noti = mocker.patch('app.celery.tasks.update_notification_status_by_id') + + create_letters_pdf(sample_letter_notification.id) + + assert mock_get_letters_pdf.called + assert mock_retry.called + assert mock_update_noti.called + mock_update_noti.assert_called_once_with(sample_letter_notification.id, 'technical-failure') From 3464336aff0b8d3d4ce12c218eb69a3a7b44eee4 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 11 Dec 2017 11:00:27 +0000 Subject: [PATCH 19/25] Refactored tasks.py to split out letters_pdf tasks - Added has_permission helper in models.py to check permission in service - Moved letters pdf tasks to separate file - Moved letters pdf tests to own file --- app/celery/letters_pdf_tasks.py | 68 +++++++++++++++ app/celery/tasks.py | 64 +------------- app/models.py | 3 + tests/app/celery/test_letters_pdf_tasks.py | 99 ++++++++++++++++++++++ tests/app/celery/test_tasks.py | 89 +------------------ 5 files changed, 176 insertions(+), 147 deletions(-) create mode 100644 app/celery/letters_pdf_tasks.py create mode 100644 tests/app/celery/test_letters_pdf_tasks.py diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py new file mode 100644 index 000000000..69e18e645 --- /dev/null +++ b/app/celery/letters_pdf_tasks.py @@ -0,0 +1,68 @@ +from flask import current_app +from requests import ( + post as requests_post, + RequestException +) + +from botocore.exceptions import ClientError as BotoClientError +from sqlalchemy.orm.exc import NoResultFound + +from app import notify_celery +from app.aws import s3 +from app.config import QueueNames +from app.dao.notifications_dao import get_notification_by_id, update_notification_status_by_id +from app.statsd_decorators import statsd + + +@notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300) +@statsd(namespace="tasks") +def create_letters_pdf(self, notification_id): + try: + notification = get_notification_by_id(notification_id) + if not notification: + raise NoResultFound() + + pdf_data = get_letters_pdf( + notification.template, + contact_block=notification.reply_to_text, + org_id=notification.service.dvla_organisation.id, + values=notification.personalisation + ) + current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format( + notification.id, notification.reference, notification.created_at, len(pdf_data))) + s3.upload_letters_pdf(reference=notification.reference, crown=True, filedata=pdf_data) + except (RequestException, BotoClientError): + try: + current_app.logger.exception( + "Letters PDF notification creation for id: {} failed".format(notification_id) + ) + self.retry(queue=QueueNames.RETRY) + except self.MaxRetriesExceededError: + current_app.logger.exception( + "RETRY FAILED: task create_letters_pdf failed for notification {}".format(notification_id), + ) + update_notification_status_by_id(notification_id, 'technical-failure') + + +def get_letters_pdf(template, contact_block, org_id, values): + template_for_letter_print = { + "subject": template.subject, + "content": template.content + } + + data = { + 'letter_contact_block': contact_block, + 'template': template_for_letter_print, + 'values': values, + 'dvla_org_id': org_id, + } + resp = requests_post( + '{}/print.pdf'.format( + current_app.config['TEMPLATE_PREVIEW_API_HOST'] + ), + json=data, + headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} + ) + resp.raise_for_status() + + return resp.content diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 9b4d2e398..f1260a6f0 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -4,7 +4,6 @@ from collections import namedtuple from celery.signals import worker_process_shutdown from flask import current_app -import requests from notifications_utils.recipients import ( RecipientCSV @@ -20,7 +19,6 @@ from requests import ( RequestException ) from sqlalchemy.exc import SQLAlchemyError -from sqlalchemy.orm.exc import NoResultFound from botocore.exceptions import ClientError as BotoClientError from app import ( @@ -32,6 +30,7 @@ from app import ( ) from app.aws import s3 from app.celery import provider_tasks +from app.celery import letters_pdf_tasks from app.celery.service_callback_tasks import send_delivery_status_to_service from app.config import QueueNames from app.dao.inbound_sms_dao import dao_get_inbound_sms_by_id @@ -48,7 +47,6 @@ from app.dao.notifications_dao import ( dao_update_notifications_by_reference, dao_get_last_notification_added_for_job_id, dao_get_notifications_by_references, - update_notification_status_by_id ) from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service @@ -127,7 +125,7 @@ def process_job(job_id): def job_complete(job, service, template_type, resumed=False, start=None): if ( template_type == LETTER_TYPE and - 'letters_as_pdf' not in [p.permission for p in service.permissions] + not service.has_permission('letters_as_pdf') ): if service.research_mode: update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) @@ -320,9 +318,9 @@ def save_letter( ) if ( - 'letters_as_pdf' in [p.permission for p in service.permissions] and not service.research_mode + service.has_permission('letters_as_pdf') and not service.research_mode ): - create_letters_pdf.apply_async( + letters_pdf_tasks.create_letters_pdf.apply_async( [str(saved_notification.id)], queue=QueueNames.CREATE_LETTERS_PDF ) @@ -596,57 +594,3 @@ def process_incomplete_job(job_id): process_row(row_number, recipient, personalisation, template, job, job.service) job_complete(job, job.service, template.template_type, resumed=True) - - -@notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300) -@statsd(namespace="tasks") -def create_letters_pdf(self, notification_id): - try: - notification = get_notification_by_id(notification_id) - if not notification: - raise NoResultFound() - - pdf_data = get_letters_pdf( - notification.template, - contact_block=notification.reply_to_text, - org_id=notification.service.dvla_organisation.id, - values=notification.personalisation - ) - current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format( - notification.id, notification.reference, notification.created_at, len(pdf_data))) - s3.upload_letters_pdf(reference=notification.reference, crown=True, filedata=pdf_data) - except (RequestException, BotoClientError): - try: - current_app.logger.exception( - "Letters PDF notification creation for id: {} failed".format(notification_id) - ) - self.retry(queue=QueueNames.RETRY) - except self.MaxRetriesExceededError: - current_app.logger.exception( - "RETRY FAILED: task create_letters_pdf failed for notification {}".format(notification_id), - ) - update_notification_status_by_id(notification_id, 'technical-failure') - - -def get_letters_pdf(template, contact_block, org_id, values=None): - template_for_letter_print = { - "subject": template.subject, - "content": template.content - } - - data = { - 'letter_contact_block': contact_block, - 'template': template_for_letter_print, - 'values': values, - 'dvla_org_id': org_id, - } - resp = requests.post( - '{}/print.pdf'.format( - current_app.config['TEMPLATE_PREVIEW_API_HOST'] - ), - json=data, - headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])} - ) - resp.raise_for_status() - - return resp.content diff --git a/app/models.py b/app/models.py index bd016b4d0..2f196c7a5 100644 --- a/app/models.py +++ b/app/models.py @@ -283,6 +283,9 @@ class Service(db.Model, Versioned): default_letter_contact = [x for x in self.letter_contacts if x.is_default] return default_letter_contact[0].contact_block if default_letter_contact else None + def has_permission(self, permission): + return permission in [p.permission for p in self.permissions] + class AnnualBilling(db.Model): __tablename__ = "annual_billing" diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py new file mode 100644 index 000000000..27f727571 --- /dev/null +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -0,0 +1,99 @@ +import pytest +import requests_mock + +from botocore.exceptions import ClientError +from celery.exceptions import MaxRetriesExceededError +from requests import RequestException +from sqlalchemy.orm.exc import NoResultFound + +from app.celery.letters_pdf_tasks import ( + create_letters_pdf, + get_letters_pdf, +) + +from tests.conftest import set_config_values + + +def test_should_have_decorated_tasks_functions(): + assert create_letters_pdf.__wrapped__.__name__ == 'create_letters_pdf' + + +@pytest.mark.parametrize('personalisation', [{'name': 'test'}, None]) +def test_get_letters_pdf_calls_notifications_template_preview_service_correctly( + notify_api, mocker, client, sample_letter_template, personalisation): + contact_block = 'Mr Foo,\n1 Test Street,\nLondon\nN1' + dvla_org_id = '002' + + with set_config_values(notify_api, { + 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', + 'TEMPLATE_PREVIEW_API_KEY': 'test-key' + }): + with requests_mock.Mocker() as request_mock: + mock_post = request_mock.post( + 'http://localhost/notifications-template-preview/print.pdf', content=b'\x00\x01', status_code=200) + + get_letters_pdf( + sample_letter_template, contact_block=contact_block, org_id=dvla_org_id, values=personalisation) + + assert mock_post.last_request.json() == { + 'values': personalisation, + 'letter_contact_block': contact_block, + 'dvla_org_id': dvla_org_id, + 'template': { + 'subject': sample_letter_template.subject, + 'content': sample_letter_template.content + } + } + + +def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notification): + mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=b'\x00\x01') + mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf') + + create_letters_pdf(sample_letter_notification.id) + + mock_s3.assert_called_with( + reference=sample_letter_notification.reference, + crown=True, + filedata=b'\x00\x01' + ) + + +def test_create_letters_pdf_non_existent_notification(notify_api, mocker, fake_uuid): + with pytest.raises(expected_exception=NoResultFound): + create_letters_pdf(fake_uuid) + + +def test_create_letters_pdf_handles_request_errors(mocker, sample_letter_notification): + mock_get_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', side_effect=RequestException) + mock_retry = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.retry') + + create_letters_pdf(sample_letter_notification.id) + + assert mock_get_letters_pdf.called + assert mock_retry.called + + +def test_create_letters_pdf_handles_s3_errors(mocker, sample_letter_notification): + mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf') + mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf', side_effect=ClientError({}, 'operation_name')) + mock_retry = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.retry') + + create_letters_pdf(sample_letter_notification.id) + + assert mock_s3.called + assert mock_retry.called + + +def test_create_letters_pdf_sets_technical_failure_max_retries(mocker, sample_letter_notification): + mock_get_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', side_effect=RequestException) + mock_retry = mocker.patch( + 'app.celery.letters_pdf_tasks.create_letters_pdf.retry', side_effect=MaxRetriesExceededError) + mock_update_noti = mocker.patch('app.celery.letters_pdf_tasks.update_notification_status_by_id') + + create_letters_pdf(sample_letter_notification.id) + + assert mock_get_letters_pdf.called + assert mock_retry.called + assert mock_update_noti.called + mock_update_noti.assert_called_once_with(sample_letter_notification.id, 'technical-failure') diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 7af5c0a06..ae892291d 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -9,9 +9,8 @@ from flask import current_app from freezegun import freeze_time from requests import RequestException from sqlalchemy.exc import SQLAlchemyError -from celery.exceptions import Retry, MaxRetriesExceededError +from celery.exceptions import Retry from botocore.exceptions import ClientError -from sqlalchemy.orm.exc import NoResultFound from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate, LetterDVLATemplate from app import (encryption, DATETIME_FORMAT) @@ -21,8 +20,6 @@ from app.celery.scheduled_tasks import check_job_status from app.celery.tasks import ( build_dvla_file, create_dvla_file_contents_for_job, - create_letters_pdf, - get_letters_pdf, job_complete, process_job, process_row, @@ -51,7 +48,6 @@ from app.models import ( SMS_TYPE ) -from tests.conftest import set_config_values from tests.app import load_example_csv from tests.app.conftest import ( sample_service as create_sample_service, @@ -99,7 +95,6 @@ def test_should_have_decorated_tasks_functions(): assert save_sms.__wrapped__.__name__ == 'save_sms' assert save_email.__wrapped__.__name__ == 'save_email' assert save_letter.__wrapped__.__name__ == 'save_letter' - assert create_letters_pdf.__wrapped__.__name__ == 'create_letters_pdf' @pytest.fixture @@ -1048,7 +1043,7 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session): def test_save_letter_saves_letter_calls_create_letters_as_pdf_with_letters_as_pdf_permission( mocker, notify_db_session, sample_letter_job): service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf') - mock_create_letters_pdf = mocker.patch('app.celery.tasks.create_letters_pdf.apply_async') + mock_create_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async') personalisation = { 'addressline1': 'Foo', @@ -1505,83 +1500,3 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template): assert mock_build_dvla.called assert mock_letter_saver.call_count == 8 - - -@pytest.mark.parametrize('personalisation', [{'name': 'test'}, None]) -def test_get_letters_pdf_calls_notifications_template_preview_service_correctly( - notify_api, mocker, client, sample_letter_template, personalisation): - contact_block = 'Mr Foo,\n1 Test Street,\nLondon\nN1' - dvla_org_id = '002' - - with set_config_values(notify_api, { - 'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview', - 'TEMPLATE_PREVIEW_API_KEY': 'test-key' - }): - with requests_mock.Mocker() as request_mock: - mock_post = request_mock.post( - 'http://localhost/notifications-template-preview/print.pdf', content=b'\x00\x01', status_code=200) - - get_letters_pdf( - sample_letter_template, contact_block=contact_block, org_id=dvla_org_id, values=personalisation) - - assert mock_post.last_request.json() == { - 'values': personalisation, - 'letter_contact_block': contact_block, - 'dvla_org_id': dvla_org_id, - 'template': { - 'subject': sample_letter_template.subject, - 'content': sample_letter_template.content - } - } - - -def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notification): - mocker.patch('app.celery.tasks.get_letters_pdf', return_value=b'\x00\x01') - mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf') - - create_letters_pdf(sample_letter_notification.id) - - mock_s3.assert_called_with( - reference=sample_letter_notification.reference, - crown=True, - filedata=b'\x00\x01' - ) - - -def test_create_letters_pdf_non_existent_notification(notify_api, mocker, fake_uuid): - with pytest.raises(expected_exception=NoResultFound): - create_letters_pdf(fake_uuid) - - -def test_create_letters_pdf_handles_request_errors(mocker, sample_letter_notification): - mock_get_letters_pdf = mocker.patch('app.celery.tasks.get_letters_pdf', side_effect=RequestException) - mock_retry = mocker.patch('app.celery.tasks.create_letters_pdf.retry') - - create_letters_pdf(sample_letter_notification.id) - - assert mock_get_letters_pdf.called - assert mock_retry.called - - -def test_create_letters_pdf_handles_s3_errors(mocker, sample_letter_notification): - mocker.patch('app.celery.tasks.get_letters_pdf') - mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf', side_effect=ClientError({}, 'operation_name')) - mock_retry = mocker.patch('app.celery.tasks.create_letters_pdf.retry') - - create_letters_pdf(sample_letter_notification.id) - - assert mock_s3.called - assert mock_retry.called - - -def test_create_letters_pdf_sets_technical_failure_max_retries(mocker, sample_letter_notification): - mock_get_letters_pdf = mocker.patch('app.celery.tasks.get_letters_pdf', side_effect=RequestException) - mock_retry = mocker.patch('app.celery.tasks.create_letters_pdf.retry', side_effect=MaxRetriesExceededError) - mock_update_noti = mocker.patch('app.celery.tasks.update_notification_status_by_id') - - create_letters_pdf(sample_letter_notification.id) - - assert mock_get_letters_pdf.called - assert mock_retry.called - assert mock_update_noti.called - mock_update_noti.assert_called_once_with(sample_letter_notification.id, 'technical-failure') From 02b67c95e5a110c6be06559b637d6488adf4ebc7 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 11 Dec 2017 16:12:07 +0000 Subject: [PATCH 20/25] Use crown flag from service --- app/celery/letters_pdf_tasks.py | 2 +- tests/app/celery/test_letters_pdf_tasks.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 69e18e645..5b675f34b 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -30,7 +30,7 @@ def create_letters_pdf(self, notification_id): ) current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format( notification.id, notification.reference, notification.created_at, len(pdf_data))) - s3.upload_letters_pdf(reference=notification.reference, crown=True, filedata=pdf_data) + s3.upload_letters_pdf(reference=notification.reference, crown=notification.service.crown, filedata=pdf_data) except (RequestException, BotoClientError): try: current_app.logger.exception( diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 27f727571..0af5c78ab 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -54,7 +54,7 @@ def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notif mock_s3.assert_called_with( reference=sample_letter_notification.reference, - crown=True, + crown=sample_letter_notification.service.crown, filedata=b'\x00\x01' ) From 3e71e9a294cfc2ef5e030f282b5469ca5a1f7291 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 11 Dec 2017 16:13:41 +0000 Subject: [PATCH 21/25] Update save_letter to handle research mode --- app/celery/tasks.py | 18 ++++++++++------- tests/app/celery/test_tasks.py | 37 +++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index f1260a6f0..8657cfb5a 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -317,13 +317,17 @@ def save_letter( reply_to_text=service.get_default_letter_contact() ) - if ( - service.has_permission('letters_as_pdf') and not service.research_mode - ): - letters_pdf_tasks.create_letters_pdf.apply_async( - [str(saved_notification.id)], - queue=QueueNames.CREATE_LETTERS_PDF - ) + if service.has_permission('letters_as_pdf'): + if not service.research_mode: + letters_pdf_tasks.create_letters_pdf.apply_async( + [str(saved_notification.id)], + queue=QueueNames.CREATE_LETTERS_PDF + ) + else: + update_letter_notifications_to_sent_to_dvla.apply_async( + kwargs={'notification_references': [saved_notification.reference]}, + queue=QueueNames.RESEARCH_MODE + ) current_app.logger.info("Letter {} created at {}".format(saved_notification.id, saved_notification.created_at)) except SQLAlchemyError as e: diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index ae892291d..a5837b3a2 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1040,7 +1040,42 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session): assert notification_db.reply_to_text == "Address contact" -def test_save_letter_saves_letter_calls_create_letters_as_pdf_with_letters_as_pdf_permission( +def test_save_letter_calls_update_noti_to_sent_task_with_letters_as_pdf_permission_in_research_mode( + mocker, notify_db_session, sample_letter_job): + sample_letter_job.service.research_mode = True + service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf') + mock_update_letter_noti_sent = mocker.patch( + 'app.celery.tasks.update_letter_notifications_to_sent_to_dvla.apply_async') + mocker.patch('app.celery.tasks.create_random_identifier', return_value="this-is-random-in-real-life") + + personalisation = { + 'addressline1': 'Foo', + 'addressline2': 'Bar', + 'postcode': 'Flob', + } + notification_json = _notification_json( + template=sample_letter_job.template, + to='Foo', + personalisation=personalisation, + job_id=sample_letter_job.id, + row_number=1 + ) + notification_id = uuid.uuid4() + + save_letter( + sample_letter_job.service_id, + notification_id, + encryption.encrypt(notification_json), + ) + + assert mock_update_letter_noti_sent.called + mock_update_letter_noti_sent.assert_called_once_with( + kwargs={'notification_references': ['this-is-random-in-real-life']}, + queue=QueueNames.RESEARCH_MODE + ) + + +def test_save_letter_calls_create_letters_pdf_task_with_letters_as_pdf_permission_and_not_in_research( mocker, notify_db_session, sample_letter_job): service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf') mock_create_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async') From d0fcfa7d0cee022f109e80a63f824412da0bf77d Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 11 Dec 2017 16:17:07 +0000 Subject: [PATCH 22/25] Update sample_service_full_perms to leave out letters_as_pdf permissions - as letters_as_pdf is a temporary service permission until the build dvla file process is deprecated rather than update all the tests to remove the permission so that they pass, lets remove it here instead --- tests/app/conftest.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index c8f06e8c4..e51aedd7d 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -161,7 +161,7 @@ def sample_service( 'message_limit': limit, 'restricted': restricted, 'email_from': email_from, - 'created_by': user + 'created_by': user, } service = Service.query.filter_by(name=service_name).first() if not service: @@ -188,7 +188,7 @@ def sample_service_full_permissions(notify_db, notify_db_session): notify_db_session, # ensure name doesn't clash with regular sample service service_name="sample service full permissions", - permissions=SERVICE_PERMISSION_TYPES + permissions=set(SERVICE_PERMISSION_TYPES) - {'letters_as_pdf'} ) @@ -299,10 +299,6 @@ def sample_template_without_email_permission(notify_db, notify_db_session): @pytest.fixture def sample_letter_template(sample_service_full_permissions): - # remove letters_as_pdf from fixture until we drop building of dvla files - from app.dao.service_permissions_dao import dao_remove_service_permission - dao_remove_service_permission(sample_service_full_permissions.id, 'letters_as_pdf') - return create_template(sample_service_full_permissions, template_type=LETTER_TYPE) From f42df8af730a0b6a696f800bda82ee014a439d77 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 11 Dec 2017 16:20:31 +0000 Subject: [PATCH 23/25] Ignore services that have letters_as_pdf permission - We don't want to process the letters as a build file for api calls when the service is generating letters as pdf. --- app/dao/notifications_dao.py | 18 ++++++++++++++- .../test_letter_api_notification_dao.py | 22 +++++++++++++++++-- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d9886fcfd..1ecef5f7c 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -26,6 +26,8 @@ from app.models import ( Notification, NotificationHistory, ScheduledNotification, + Service, + ServicePermission, Template, TemplateHistory, KEY_TYPE_NORMAL, @@ -541,14 +543,28 @@ def dao_set_created_live_letter_api_notifications_to_pending(): this is used in the run_scheduled_jobs task, so we put a FOR UPDATE lock on the job table for the duration of the transaction so that if the task is run more than once concurrently, one task will block the other select from completing until it commits. + + Note - do not process services that have letters_as_pdf permission as they + will get processed when the letters PDF zip task is created """ + + # Ignore services that have letters_as_pdf permission + services_without_letters_as_pdf = [ + s.id for s in Service.query.filter( + ~Service.permissions.any( + ServicePermission.permission == 'letters_as_pdf' + ) + ).all() + ] + notifications = db.session.query( Notification ).filter( Notification.notification_type == LETTER_TYPE, Notification.status == NOTIFICATION_CREATED, Notification.key_type == KEY_TYPE_NORMAL, - Notification.api_key != None # noqa + Notification.api_key != None, # noqa + Notification.service_id.in_(services_without_letters_as_pdf) ).with_for_update( ).all() diff --git a/tests/app/dao/notification_dao/test_letter_api_notification_dao.py b/tests/app/dao/notification_dao/test_letter_api_notification_dao.py index 4a624f3eb..96f79ceff 100644 --- a/tests/app/dao/notification_dao/test_letter_api_notification_dao.py +++ b/tests/app/dao/notification_dao/test_letter_api_notification_dao.py @@ -1,13 +1,15 @@ from app.models import ( + Notification, NOTIFICATION_CREATED, NOTIFICATION_PENDING, NOTIFICATION_SENDING, KEY_TYPE_TEST, - KEY_TYPE_NORMAL + KEY_TYPE_NORMAL, + LETTER_TYPE ) from app.dao.notifications_dao import dao_set_created_live_letter_api_notifications_to_pending -from tests.app.db import create_notification +from tests.app.db import create_notification, create_service, create_template def test_should_only_get_letter_notifications( @@ -23,6 +25,22 @@ def test_should_only_get_letter_notifications( assert ret == [sample_letter_notification] +def test_should_ignore_letters_as_pdf( + sample_letter_notification, +): + service = create_service(service_permissions=[LETTER_TYPE, 'letters_as_pdf']) + template = create_template(service, template_type=LETTER_TYPE) + create_notification(template) + + all_noti = Notification.query.all() + assert len(all_noti) == 2 + + ret = dao_set_created_live_letter_api_notifications_to_pending() + + assert sample_letter_notification.status == NOTIFICATION_PENDING + assert ret == [sample_letter_notification] + + def test_should_only_get_created_letters(sample_letter_template): created_noti = create_notification(sample_letter_template, status=NOTIFICATION_CREATED) create_notification(sample_letter_template, status=NOTIFICATION_PENDING) From ab72d6f555fa25226e82a531bf9d2a6f692f2822 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 11 Dec 2017 16:23:09 +0000 Subject: [PATCH 24/25] Call the create letters pdf task in api calls for services with letters as pdf --- app/v2/notifications/post_notifications.py | 7 ++++++ .../test_post_letter_notifications.py | 25 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 08847f2cb..ed9d67c7f 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -14,6 +14,7 @@ from app.models import ( NOTIFICATION_CREATED, NOTIFICATION_SENDING ) +from app.celery.letters_pdf_tasks import create_letters_pdf from app.celery.tasks import update_letter_notifications_to_sent_to_dvla from app.notifications.process_notifications import ( persist_notification, @@ -187,6 +188,12 @@ def process_letter_notification(*, letter_data, api_key, template): queue=QueueNames.RESEARCH_MODE ) + if api_key.service.has_permission('letters_as_pdf'): + create_letters_pdf.apply_async( + [str(notification.id)], + queue=QueueNames.CREATE_LETTERS_PDF + ) + return notification diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index d6eca3c8d..b433f83cd 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -4,6 +4,8 @@ from flask import json from flask import url_for import pytest +from app.config import QueueNames +from app.dao import service_permissions_dao from app.models import EMAIL_TYPE from app.models import Job from app.models import KEY_TYPE_NORMAL @@ -80,6 +82,29 @@ def test_post_letter_notification_returns_201(client, sample_letter_template, mo assert not notification.reply_to_text +def test_post_letter_notification_for_letters_as_pdf_calls_celery_task(client, sample_letter_template, mocker): + service_permissions_dao.dao_add_service_permission(sample_letter_template.service.id, 'letters_as_pdf') + + data = { + 'template_id': str(sample_letter_template.id), + 'personalisation': { + 'address_line_1': 'Her Royal Highness Queen Elizabeth II', + 'address_line_2': 'Buckingham Palace', + 'address_line_3': 'London', + 'postcode': 'SW1 1AA', + 'name': 'Lizzie' + }, + 'reference': 'foo' + } + fake_task = mocker.patch('app.celery.tasks.letters_pdf_tasks.create_letters_pdf.apply_async') + + letter_request(client, data, service_id=sample_letter_template.service_id) + + notification = Notification.query.one() + + fake_task.assert_called_once_with([str(notification.id)], queue=QueueNames.CREATE_LETTERS_PDF) + + def test_post_letter_notification_returns_400_and_missing_template( client, sample_service_full_permissions From da93ba296eb58334912da113e5905317dc73716e Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 12 Dec 2017 11:56:42 +0000 Subject: [PATCH 25/25] Refactored notifications_dao - Introduce a `_raise` flag for `get_notification_by_id` so that sql alchemy will raise the NoResults error rather than the app - Refactor `dao_set_created_live_letter_api_notifications_to_pending` to use a join for getting services that don't have `letters_as_pdf` as marginally faster. --- app/celery/letters_pdf_tasks.py | 5 +--- app/dao/notifications_dao.py | 24 +++++++++---------- .../test_letter_api_notification_dao.py | 4 +--- 3 files changed, 13 insertions(+), 20 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 5b675f34b..24c7347ae 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -5,7 +5,6 @@ from requests import ( ) from botocore.exceptions import ClientError as BotoClientError -from sqlalchemy.orm.exc import NoResultFound from app import notify_celery from app.aws import s3 @@ -18,9 +17,7 @@ from app.statsd_decorators import statsd @statsd(namespace="tasks") def create_letters_pdf(self, notification_id): try: - notification = get_notification_by_id(notification_id) - if not notification: - raise NoResultFound() + notification = get_notification_by_id(notification_id, _raise=True) pdf_data = get_letters_pdf( notification.template, diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 1ecef5f7c..29b49ba7c 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -226,8 +226,11 @@ def get_notification_with_personalisation(service_id, notification_id, key_type) @statsd(namespace="dao") -def get_notification_by_id(notification_id): - return Notification.query.filter_by(id=notification_id).first() +def get_notification_by_id(notification_id, _raise=False): + if _raise: + return Notification.query.filter_by(id=notification_id).one() + else: + return Notification.query.filter_by(id=notification_id).first() def get_notifications(filter_dict=None): @@ -547,24 +550,19 @@ def dao_set_created_live_letter_api_notifications_to_pending(): Note - do not process services that have letters_as_pdf permission as they will get processed when the letters PDF zip task is created """ - - # Ignore services that have letters_as_pdf permission - services_without_letters_as_pdf = [ - s.id for s in Service.query.filter( - ~Service.permissions.any( - ServicePermission.permission == 'letters_as_pdf' - ) - ).all() - ] - notifications = db.session.query( Notification + ).join( + Service ).filter( Notification.notification_type == LETTER_TYPE, Notification.status == NOTIFICATION_CREATED, Notification.key_type == KEY_TYPE_NORMAL, Notification.api_key != None, # noqa - Notification.service_id.in_(services_without_letters_as_pdf) + # Ignore services that have letters_as_pdf permission + ~Service.permissions.any( + ServicePermission.permission == 'letters_as_pdf' + ) ).with_for_update( ).all() diff --git a/tests/app/dao/notification_dao/test_letter_api_notification_dao.py b/tests/app/dao/notification_dao/test_letter_api_notification_dao.py index 96f79ceff..c52de01e1 100644 --- a/tests/app/dao/notification_dao/test_letter_api_notification_dao.py +++ b/tests/app/dao/notification_dao/test_letter_api_notification_dao.py @@ -25,9 +25,7 @@ def test_should_only_get_letter_notifications( assert ret == [sample_letter_notification] -def test_should_ignore_letters_as_pdf( - sample_letter_notification, -): +def test_should_ignore_letters_as_pdf(sample_letter_notification): service = create_service(service_permissions=[LETTER_TYPE, 'letters_as_pdf']) template = create_template(service, template_type=LETTER_TYPE) create_notification(template)