From 2ed50e760fc5bcf5d5ddb80710ae2d2b9e45d237 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 9 Oct 2018 13:27:49 +0100 Subject: [PATCH 01/11] Revert "Celery 4" --- Makefile | 33 +++++++++------------------------ app/config.py | 18 +++++++++++++----- app/dao/notifications_dao.py | 2 +- docker/Dockerfile | 1 - requirements-app.txt | 4 ++-- requirements.txt | 15 +++++++-------- scripts/bootstrap.sh | 11 ++++++++++- 7 files changed, 42 insertions(+), 42 deletions(-) diff --git a/Makefile b/Makefile index e94889cea..3612540bc 100644 --- a/Makefile +++ b/Makefile @@ -2,6 +2,7 @@ SHELL := /bin/bash DATE = $(shell date +%Y-%m-%d:%H:%M:%S) +PIP_ACCEL_CACHE ?= ${CURDIR}/cache/pip-accel APP_VERSION_FILE = app/version.py GIT_BRANCH ?= $(shell git symbolic-ref --short HEAD 2> /dev/null || echo "detached") @@ -33,19 +34,11 @@ help: @cat $(MAKEFILE_LIST) | grep -E '^[a-zA-Z_-]+:.*?## .*$$' | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}' .PHONY: venv -# if there's a virtualenv already active, we don't want to do anything -ifeq ($(VIRTUAL_ENV),) venv: venv/bin/activate ## Create virtualenv if it does not exist venv/bin/activate: test -d venv || virtualenv venv -p python3 - -VENV_BIN_DIR = $(shell . venv/bin/activate && echo $$VIRTUAL_ENV/bin) -else -venv: - -VENV_BIN_DIR = $(VIRTUAL_ENV)/bin -endif + . venv/bin/activate && pip install pip-accel .PHONY: check-env-vars check-env-vars: ## Check mandatory environment variables @@ -72,9 +65,9 @@ production: ## Set environment to production @true .PHONY: dependencies -dependencies: venv - $(call install-pycurl, $(VENV_BIN_DIR)/pip) - $(VENV_BIN_DIR)/pip install -r requirements_for_test.txt +dependencies: venv ## Install build dependencies + mkdir -p ${PIP_ACCEL_CACHE} + . venv/bin/activate && PIP_ACCEL_CACHE=${PIP_ACCEL_CACHE} pip-accel install -r requirements_for_test.txt .PHONY: generate-version-file generate-version-file: ## Generates the app version file @@ -82,7 +75,7 @@ generate-version-file: ## Generates the app version file .PHONY: build build: dependencies generate-version-file ## Build project - + . venv/bin/activate && PIP_ACCEL_CACHE=${PIP_ACCEL_CACHE} pip-accel install -r requirements.txt .PHONY: build-paas-artifact build-paas-artifact: ## Build the deploy artifact for PaaS @@ -104,7 +97,6 @@ test: venv generate-version-file ## Run tests freeze-requirements: rm -rf venv-freeze virtualenv -p python3 venv-freeze - $(call install-pycurl, $$(pwd)/venv-freeze/bin/pip) $$(pwd)/venv-freeze/bin/pip install -r requirements-app.txt echo '# pyup: ignore file' > requirements.txt echo '# This file is autogenerated. Do not edit it manually.' >> requirements.txt @@ -113,15 +105,6 @@ freeze-requirements: $$(pwd)/venv-freeze/bin/pip freeze -r <(sed '/^--/d' requirements-app.txt) | sed -n '/The following requirements were added by pip freeze/,$$p' >> requirements.txt rm -rf venv-freeze -define install-pycurl - # install pycurl separately to avoid flags disabling wheels for other packages - PYCURL_SSL_LIBRARY=openssl ${1} install pycurl==7.43.0.2 --global-option="build_ext" --global-option="-I/usr/local/opt/openssl/include" -endef - -.PHONY: install-pycurl -install-pycurl: - $(call install-pycurl, pip) - .PHONY: test-requirements test-requirements: @diff requirements-app.txt requirements.txt | grep '<' \ @@ -131,10 +114,11 @@ test-requirements: .PHONY: coverage coverage: venv ## Create coverage report - coveralls + . venv/bin/activate && coveralls .PHONY: prepare-docker-build-image prepare-docker-build-image: ## Prepare the Docker builder image + mkdir -p ${PIP_ACCEL_CACHE} make -C docker build .PHONY: build-with-docker @@ -142,6 +126,7 @@ build-with-docker: prepare-docker-build-image ## Build inside a Docker container @docker run -i${DOCKER_TTY} --rm \ --name "${DOCKER_CONTAINER_PREFIX}-build" \ -v "`pwd`:/var/project" \ + -v "${PIP_ACCEL_CACHE}:/var/project/cache/pip-accel" \ -e UID=$(shell id -u) \ -e GID=$(shell id -g) \ -e GIT_COMMIT=${GIT_COMMIT} \ diff --git a/app/config.py b/app/config.py index c37dd5d00..ae7788d34 100644 --- a/app/config.py +++ b/app/config.py @@ -150,9 +150,9 @@ class Config(object): BROKER_URL = 'sqs://' BROKER_TRANSPORT_OPTIONS = { 'region': AWS_REGION, + 'polling_interval': 1, # 1 second 'visibility_timeout': 310, - 'queue_name_prefix': NOTIFICATION_QUEUE_PREFIX, - 'wait_time_seconds': 20 # enable long polling, with a wait time of 20 seconds + 'queue_name_prefix': NOTIFICATION_QUEUE_PREFIX } CELERY_ENABLE_UTC = True CELERY_TIMEZONE = 'Europe/London' @@ -270,9 +270,7 @@ class Config(object): 'options': {'queue': QueueNames.PERIODIC} } } - - # this is overriden by the -Q command, but locally, we should read from all queues - CELERY_QUEUES = [Queue(queue, Exchange('default'), routing_key=queue) for queue in QueueNames.all_queues()] + CELERY_QUEUES = [] NOTIFICATIONS_ALERT = 5 # five mins FROM_NUMBER = 'development' @@ -360,6 +358,11 @@ class Development(Config): STATSD_PORT = 1000 STATSD_PREFIX = "stats-prefix" + for queue in QueueNames.all_queues(): + Config.CELERY_QUEUES.append( + Queue(queue, Exchange('default'), routing_key=queue) + ) + API_HOST_NAME = "http://localhost:6011" API_RATE_LIMIT_ENABLED = True @@ -382,6 +385,11 @@ class Test(Development): BROKER_URL = 'you-forgot-to-mock-celery-in-your-tests://' + for queue in QueueNames.all_queues(): + Config.CELERY_QUEUES.append( + Queue(queue, Exchange('default'), routing_key=queue) + ) + API_RATE_LIMIT_ENABLED = True API_HOST_NAME = "http://localhost:6011" diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 6cfca54a8..95896e4da 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -5,7 +5,7 @@ from datetime import ( timedelta, ) -from botocore.exceptions import ClientError as BotoClientError +from boto.exception import BotoClientError from flask import current_app from notifications_utils.recipients import ( diff --git a/docker/Dockerfile b/docker/Dockerfile index db1bc6202..1060824cf 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -22,7 +22,6 @@ RUN \ libffi-dev \ python-dev \ jq \ - libcurl4-openssl-dev \ && echo "Clean up" \ && rm -rf /var/lib/apt/lists/* /tmp/* diff --git a/requirements-app.txt b/requirements-app.txt index 085306c17..5aeebe34f 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -2,7 +2,7 @@ # with package version changes made in requirements-app.txt cffi==1.11.5 -celery[sqs]==4.2.1 +celery==3.1.26.post2 # pyup: <4 docopt==0.6.2 Flask-Bcrypt==0.7.1 flask-marshmallow==0.9.0 @@ -29,4 +29,4 @@ botocore<1.11.0 git+https://github.com/alphagov/notifications-utils.git@30.5.3#egg=notifications-utils==30.5.3 -# if you want to update pycurl please do so in makefile +git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/requirements.txt b/requirements.txt index 89d555659..4a8422f37 100644 --- a/requirements.txt +++ b/requirements.txt @@ -4,7 +4,7 @@ # with package version changes made in requirements-app.txt cffi==1.11.5 -celery[sqs]==4.2.1 +celery==3.1.26.post2 # pyup: <4 docopt==0.6.2 Flask-Bcrypt==0.7.1 flask-marshmallow==0.9.0 @@ -31,15 +31,16 @@ botocore<1.11.0 git+https://github.com/alphagov/notifications-utils.git@30.5.3#egg=notifications-utils==30.5.3 -# if you want to update pycurl please do so in makefile +git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 ## The following requirements were added by pip freeze: alembic==1.0.0 -amqp==2.3.2 +amqp==1.4.9 +anyjson==0.3.3 bcrypt==3.1.4 -billiard==3.5.0.4 +billiard==3.3.0.23 bleach==2.1.3 -boto3==1.9.17 +boto3==1.6.16 certifi==2018.8.24 chardet==3.0.4 Click==7.0 @@ -53,7 +54,7 @@ idna==2.7 itsdangerous==0.24 Jinja2==2.10 jmespath==0.9.3 -kombu==4.2.1 +kombu==3.0.37 Mako==1.0.7 MarkupSafe==1.0 mistune==0.8.3 @@ -62,7 +63,6 @@ orderedset==2.0.1 phonenumbers==8.9.4 pyasn1==0.4.4 pycparser==2.19 -pycurl==7.43.0.2 PyPDF2==1.26.0 python-dateutil==2.7.3 python-editor==1.0.3 @@ -77,6 +77,5 @@ six==1.11.0 smartypants==2.0.1 statsd==3.2.2 urllib3==1.23 -vine==1.1.4 webencodings==0.5.1 Werkzeug==0.14.1 diff --git a/scripts/bootstrap.sh b/scripts/bootstrap.sh index b92832233..c46d03aa6 100755 --- a/scripts/bootstrap.sh +++ b/scripts/bootstrap.sh @@ -20,7 +20,16 @@ function display_result { fi } -make build +if [ ! $VIRTUAL_ENV ]; then + virtualenv -p python3 ./venv + . ./venv/bin/activate +fi + +# we need the version file to exist otherwise the app will blow up +make generate-version-file + +# Install Python development dependencies +pip3 install -r requirements_for_test.txt # Create Postgres databases createdb notification_api From 8af5cbab86ba39075ac58fb48ca02f33b1ab8b3b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 9 Oct 2018 14:57:28 +0100 Subject: [PATCH 02/11] Bump utils to 30.5.4 --- requirements-app.txt | 2 +- requirements.txt | 2 +- tests/app/user/test_rest_verify.py | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/requirements-app.txt b/requirements-app.txt index 5aeebe34f..f55b54765 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -27,6 +27,6 @@ awscli==1.15.82 awscli-cwlogs>=1.4,<1.5 botocore<1.11.0 -git+https://github.com/alphagov/notifications-utils.git@30.5.3#egg=notifications-utils==30.5.3 +git+https://github.com/alphagov/notifications-utils.git@30.5.4#egg=notifications-utils==30.5.4 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/requirements.txt b/requirements.txt index 4a8422f37..e848c5cf2 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,7 +29,7 @@ awscli==1.15.82 awscli-cwlogs>=1.4,<1.5 botocore<1.11.0 -git+https://github.com/alphagov/notifications-utils.git@30.5.3#egg=notifications-utils==30.5.3 +git+https://github.com/alphagov/notifications-utils.git@30.5.4#egg=notifications-utils==30.5.4 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index b97f4ef81..9d8ccee94 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -346,15 +346,15 @@ def test_reset_failed_login_count_returns_404_when_user_does_not_exist(client): @pytest.mark.parametrize('data, expected_auth_url', ( ( {}, - 'http://localhost:6012/email-auth/.', + 'http://localhost:6012/email-auth/%2E', ), ( {'to': None}, - 'http://localhost:6012/email-auth/.', + 'http://localhost:6012/email-auth/%2E', ), ( {'to': None, 'email_auth_link_host': 'https://example.com'}, - 'https://example.com/email-auth/.', + 'https://example.com/email-auth/%2E', ), )) def test_send_user_email_code( From 8f9ee7f7ad751a3d785e1063c7bbf52b4c8a8de2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 12 Oct 2018 10:36:41 +0100 Subject: [PATCH 03/11] Add letter org for Brighton and Hove --- .../versions/0236_another_letter_org.py | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 migrations/versions/0236_another_letter_org.py diff --git a/migrations/versions/0236_another_letter_org.py b/migrations/versions/0236_another_letter_org.py new file mode 100644 index 000000000..c49b8fd46 --- /dev/null +++ b/migrations/versions/0236_another_letter_org.py @@ -0,0 +1,35 @@ +"""empty message + +Revision ID: 0236_another_letter_org +Revises: 0235_add_postage_to_pk + +""" + +# revision identifiers, used by Alembic. +revision = '0236_another_letter_org' +down_revision = '0235_add_postage_to_pk' + +from alembic import op + + +NEW_ORGANISATIONS = [ + ('514', 'Brighton and Hove city council'), +] + + +def upgrade(): + for numeric_id, name in NEW_ORGANISATIONS: + op.execute(""" + INSERT + INTO dvla_organisation + VALUES ('{}', '{}') + """.format(numeric_id, name)) + + +def downgrade(): + for numeric_id, _ in NEW_ORGANISATIONS: + op.execute(""" + DELETE + FROM dvla_organisation + WHERE id = '{}' + """.format(numeric_id)) From 52095c9c8cdb55a792d5adac6250196ee1ecfe13 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 11 Oct 2018 13:40:28 +0100 Subject: [PATCH 04/11] Add filename to dvla_organisation table Added a filename column to the dvla_organisation table and populated it with the filenames that are currently hard-coded in template-preview. The filenames for letter logos are going to be stored in the database, instead of in template-preview. --- app/models.py | 1 + .../versions/0237_add_filename_to_dvla_org.py | 56 +++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 migrations/versions/0237_add_filename_to_dvla_org.py diff --git a/app/models.py b/app/models.py index 6c11689ab..4c961d419 100644 --- a/app/models.py +++ b/app/models.py @@ -247,6 +247,7 @@ class DVLAOrganisation(db.Model): __tablename__ = 'dvla_organisation' id = db.Column(db.String, primary_key=True) name = db.Column(db.String(255), nullable=True) + filename = db.Column(db.String(255), nullable=True) INTERNATIONAL_SMS_TYPE = 'international_sms' diff --git a/migrations/versions/0237_add_filename_to_dvla_org.py b/migrations/versions/0237_add_filename_to_dvla_org.py new file mode 100644 index 000000000..9e2dbbc66 --- /dev/null +++ b/migrations/versions/0237_add_filename_to_dvla_org.py @@ -0,0 +1,56 @@ +""" + +Revision ID: 0237_add_filename_to_dvla_org +Revises: 0236_another_letter_org +Create Date: 2018-09-28 15:39:21.115358 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.sql import text + + +revision = '0237_add_filename_to_dvla_org' +down_revision = '0236_another_letter_org' + + +LOGOS = { + '001': 'hm-government', + '002': 'opg', + '003': 'dwp', + '004': 'geo', + '005': 'ch', + '006': 'dwp-welsh', + '007': 'dept-for-communities', + '008': 'mmo', + '009': 'hmpo', + '500': 'hm-land-registry', + '501': 'ea', + '502': 'wra', + '503': 'eryc', + '504': 'rother', + '505': 'cadw', + '506': 'twfrs', + '507': 'thames-valley-police', + '508': 'ofgem', + '509': 'hackney', + '510': 'pension-wise', + '511': 'nhs', + '512': 'vale-of-glamorgan', + '513': 'wdc', + '514': 'brighton-hove', +} + + +def upgrade(): + conn = op.get_bind() + op.add_column('dvla_organisation', sa.Column('filename', sa.String(length=255), nullable=True)) + + for org_id, org_filename in LOGOS.items(): + conn.execute(text(""" + UPDATE dvla_organisation SET filename = :filename WHERE id = :id + """), filename=org_filename, id=org_id) + + +def downgrade(): + op.drop_column('dvla_organisation', 'filename') From e22e7245fe7fd3e34541f9b3b8345df09f35b6c2 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 3 Sep 2018 13:24:51 +0100 Subject: [PATCH 05/11] Use sanitised pdfs for sending and handle invalid pdfs, details below: - pass new, sanitised pdf for sending - move invalid pdfs to a newly created bucket - set status fro notifications that failed pdf validation to a new status validation-failed - adjust existing tests --- app/celery/letters_pdf_tasks.py | 19 +++++++------ app/config.py | 6 ++++ app/letters/utils.py | 6 ++++ app/models.py | 3 ++ .../versions/0238_add_validation_failed.py | 28 +++++++++++++++++++ tests/app/celery/test_letters_pdf_tasks.py | 8 +++--- .../notifications/test_get_notifications.py | 2 +- .../test_notification_schemas.py | 4 +-- 8 files changed, 61 insertions(+), 15 deletions(-) create mode 100644 migrations/versions/0238_add_validation_failed.py diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 595f617d7..f776383a4 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -38,9 +38,11 @@ from app.models import ( NOTIFICATION_DELIVERED, NOTIFICATION_VIRUS_SCAN_FAILED, NOTIFICATION_TECHNICAL_FAILURE, - # NOTIFICATION_VALIDATION_FAILED + NOTIFICATION_VALIDATION_FAILED ) +from app.letters.utils import move_scan_to_invalid_pdf_bucket + @notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300) @statsd(namespace="tasks") @@ -187,19 +189,18 @@ def process_virus_scan_passed(self, filename): if not new_pdf: current_app.logger.info('Invalid precompiled pdf received {} ({})'.format(notification.id, filename)) - # update_notification_status_by_id(notification.id, NOTIFICATION_VALIDATION_FAILED) - # move_scan_to_invalid_pdf_bucket() # TODO: implement this (and create bucket etc) - # scan_pdf_object.delete() - # return + update_notification_status_by_id(notification.id, NOTIFICATION_VALIDATION_FAILED) + move_scan_to_invalid_pdf_bucket(scan_pdf_object) + scan_pdf_object.delete() + return else: current_app.logger.info( "Validation was successful for precompiled pdf {} ({})".format(notification.id, filename)) current_app.logger.info('notification id {} ({}) sanitised and ready to send'.format(notification.id, filename)) - # temporarily upload original pdf while testing sanitise flow. _upload_pdf_to_test_or_live_pdf_bucket( - old_pdf, # TODO: change to new_pdf + new_pdf, filename, is_test_letter=is_test_key) @@ -237,7 +238,9 @@ def _sanitise_precomiled_pdf(self, notification, precompiled_pdf): return resp.content except RequestException as ex: if ex.response is not None and ex.response.status_code == 400: - # validation error + current_app.logger.exception( + "sanitise_precomiled_pdf validation error for notification: {}".format(notification.id) + ) return None try: diff --git a/app/config.py b/app/config.py index ae7788d34..3f6b077f5 100644 --- a/app/config.py +++ b/app/config.py @@ -337,6 +337,7 @@ class Development(Config): DVLA_RESPONSE_BUCKET_NAME = 'notify.tools-ftp' LETTERS_PDF_BUCKET_NAME = 'development-letters-pdf' LETTERS_SCAN_BUCKET_NAME = 'development-letters-scan' + INVALID_PDF_BUCKET_NAME = 'development-letters-invalid-pdf' ADMIN_CLIENT_SECRET = 'dev-notify-secret-key' SECRET_KEY = 'dev-notify-secret-key' @@ -379,6 +380,7 @@ class Test(Development): DVLA_RESPONSE_BUCKET_NAME = 'test.notify.com-ftp' LETTERS_PDF_BUCKET_NAME = 'test-letters-pdf' LETTERS_SCAN_BUCKET_NAME = 'test-letters-scan' + INVALID_PDF_BUCKET_NAME = 'test-letters-invalid-pdf' # this is overriden in jenkins and on cloudfoundry SQLALCHEMY_DATABASE_URI = os.getenv('SQLALCHEMY_DATABASE_URI', 'postgresql://localhost/test_notification_api') @@ -407,6 +409,7 @@ class Preview(Config): DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp' LETTERS_PDF_BUCKET_NAME = 'preview-letters-pdf' LETTERS_SCAN_BUCKET_NAME = 'preview-letters-scan' + INVALID_PDF_BUCKET_NAME = 'preview-letters-invalid-pdf' FROM_NUMBER = 'preview' API_RATE_LIMIT_ENABLED = True CHECK_PROXY_HEADER = False @@ -421,6 +424,7 @@ class Staging(Config): DVLA_RESPONSE_BUCKET_NAME = 'staging-notify.works-ftp' LETTERS_PDF_BUCKET_NAME = 'staging-letters-pdf' LETTERS_SCAN_BUCKET_NAME = 'staging-letters-scan' + INVALID_PDF_BUCKET_NAME = 'staging-letters-invalid-pdf' STATSD_ENABLED = True FROM_NUMBER = 'stage' API_RATE_LIMIT_ENABLED = True @@ -437,6 +441,7 @@ class Live(Config): DVLA_RESPONSE_BUCKET_NAME = 'notifications.service.gov.uk-ftp' LETTERS_PDF_BUCKET_NAME = 'production-letters-pdf' LETTERS_SCAN_BUCKET_NAME = 'production-letters-scan' + INVALID_PDF_BUCKET_NAME = 'production-letters-invalid-pdf' STATSD_ENABLED = True FROM_NUMBER = 'GOVUK' FUNCTIONAL_TEST_PROVIDER_SERVICE_ID = '6c1d81bb-dae2-4ee9-80b0-89a4aae9f649' @@ -460,6 +465,7 @@ class Sandbox(CloudFoundryConfig): DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp' LETTERS_PDF_BUCKET_NAME = 'cf-sandbox-letters-pdf' LETTERS_SCAN_BUCKET_NAME = 'cf-sandbox-letters-scan' + INVALID_PDF_BUCKET_NAME = 'cf-sandbox-letters-invalid-pdf' FROM_NUMBER = 'sandbox' REDIS_ENABLED = False diff --git a/app/letters/utils.py b/app/letters/utils.py index a3c795e5b..46ff1c4c7 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -107,6 +107,12 @@ def move_error_pdf_to_scan_bucket(source_filename): _move_s3_object(scan_bucket, error_file, scan_bucket, source_filename) +def move_scan_to_invalid_pdf_bucket(source_filename): + invalid_pdf_bucket = current_app.config['INVALID_PDF_BUCKET_NAME'] + invalid_pdf = source_filename + _move_s3_object(invalid_pdf_bucket, invalid_pdf, invalid_pdf_bucket, source_filename) + + def get_file_names_from_error_bucket(): s3 = boto3.resource('s3') scan_bucket = current_app.config['LETTERS_SCAN_BUCKET_NAME'] diff --git a/app/models.py b/app/models.py index 4c961d419..3cb89e4d5 100644 --- a/app/models.py +++ b/app/models.py @@ -1053,6 +1053,7 @@ NOTIFICATION_TECHNICAL_FAILURE = 'technical-failure' NOTIFICATION_TEMPORARY_FAILURE = 'temporary-failure' NOTIFICATION_PERMANENT_FAILURE = 'permanent-failure' NOTIFICATION_PENDING_VIRUS_CHECK = 'pending-virus-check' +NOTIFICATION_VALIDATION_FAILED = 'validation-failed' NOTIFICATION_VIRUS_SCAN_FAILED = 'virus-scan-failed' NOTIFICATION_RETURNED_LETTER = 'returned-letter' @@ -1060,6 +1061,7 @@ NOTIFICATION_STATUS_TYPES_FAILED = [ NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE, + NOTIFICATION_VALIDATION_FAILED, NOTIFICATION_VIRUS_SCAN_FAILED, NOTIFICATION_RETURNED_LETTER, ] @@ -1101,6 +1103,7 @@ NOTIFICATION_STATUS_TYPES = [ NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE, NOTIFICATION_PENDING_VIRUS_CHECK, + NOTIFICATION_VALIDATION_FAILED, NOTIFICATION_VIRUS_SCAN_FAILED, NOTIFICATION_RETURNED_LETTER, ] diff --git a/migrations/versions/0238_add_validation_failed.py b/migrations/versions/0238_add_validation_failed.py new file mode 100644 index 000000000..1dd2222af --- /dev/null +++ b/migrations/versions/0238_add_validation_failed.py @@ -0,0 +1,28 @@ +""" + +Revision ID: 0238_add_validation_failed +Revises: 0237_add_filename_to_dvla_org +Create Date: 2018-09-03 11:24:58.773824 + +""" +from alembic import op +import sqlalchemy as sa + + +revision = '0238_add_validation_failed' +down_revision = '0237_add_filename_to_dvla_org' + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.execute("INSERT INTO notification_status_types (name) VALUES ('validation-failed')") + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.execute("UPDATE notifications SET notification_status = 'permanent-failure' WHERE notification_status = 'validation-failed'") + op.execute("UPDATE notification_history SET notification_status = 'permanent-failure' WHERE notification_status = 'validation-failed'") + + op.execute("DELETE FROM notification_status_types WHERE name = 'validation-failed'") + # ### end Alembic commands ### diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 53c54d37e..ee770cc99 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -354,16 +354,16 @@ def test_process_letter_task_check_virus_scan_passed( sample_letter_notification.status = 'pending-virus-check' sample_letter_notification.key_type = key_type mock_s3upload = mocker.patch('app.celery.letters_pdf_tasks.s3upload') - mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precomiled_pdf') + mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precomiled_pdf', return_value="success") process_virus_scan_passed(filename) assert sample_letter_notification.status == noti_status mock_s3upload.assert_called_once_with( - filedata=b'pdf_content', - region='eu-west-1', bucket_name=target_bucket_name, - file_location=destination_folder + filename + filedata="success", + file_location=destination_folder + filename, + region='eu-west-1', ) mock_sanitise.assert_called_once_with( ANY, diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 4db097ef3..8bbe13db5 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -437,7 +437,7 @@ def test_get_all_notifications_filter_by_status_invalid_status(client, sample_no assert len(json_response['errors']) == 1 assert json_response['errors'][0]['message'] == "status elephant is not one of [cancelled, created, sending, " \ "sent, delivered, pending, failed, technical-failure, temporary-failure, permanent-failure, " \ - "pending-virus-check, virus-scan-failed, returned-letter, accepted, received]" + "pending-virus-check, validation-failed, virus-scan-failed, returned-letter, accepted, received]" def test_get_all_notifications_filter_by_multiple_statuses(client, sample_template): diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index cc2765789..006c3f4d3 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -44,7 +44,7 @@ def test_get_notifications_request_invalid_statuses( partial_error_status = "is not one of " \ "[cancelled, created, sending, sent, delivered, pending, failed, " \ "technical-failure, temporary-failure, permanent-failure, pending-virus-check, " \ - "virus-scan-failed, returned-letter, accepted, received]" + "validation-failed, virus-scan-failed, returned-letter, accepted, received]" with pytest.raises(ValidationError) as e: validate({'status': invalid_statuses + valid_statuses}, get_notifications_request) @@ -92,7 +92,7 @@ def test_get_notifications_request_invalid_statuses_and_template_types(): for invalid_status in ["elephant", "giraffe"]: assert "status {} is not one of [cancelled, created, sending, sent, delivered, " \ "pending, failed, technical-failure, temporary-failure, permanent-failure, " \ - "pending-virus-check, virus-scan-failed, returned-letter, accepted, received]".format( + "pending-virus-check, validation-failed, virus-scan-failed, returned-letter, accepted, received]".format( invalid_status ) in error_messages From 9606513e1f0a19bf16d282353cd9b555a038a42d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 15 Oct 2018 16:07:22 +0100 Subject: [PATCH 06/11] exclude CYSP from sanitise we know their content is already good, but they slightly exceed the margins. Until they deploy their fixed template, lets not valiate them --- app/celery/letters_pdf_tasks.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index f776383a4..c3a47608d 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -187,6 +187,11 @@ def process_virus_scan_passed(self, filename): new_pdf = _sanitise_precomiled_pdf(self, notification, old_pdf) + # TODO: Remove this once CYSP update their template to not cross over the margins + if notification.service_id == 'fe44178f-3b45-4625-9f85-2264a36dd9ec': # CYSP + # Check your state pension submit letters with good addresses and notify tags, so just use their supplied pdf + new_pdf = old_pdf + if not new_pdf: current_app.logger.info('Invalid precompiled pdf received {} ({})'.format(notification.id, filename)) update_notification_status_by_id(notification.id, NOTIFICATION_VALIDATION_FAILED) From 7bf68e3664f7e9e4676c086a440f56796bcd79ed Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 16 Oct 2018 17:20:34 +0100 Subject: [PATCH 07/11] fix failed sanitise flow the move from virus scan to validation failed function was called with the wrong variables, and had some internal logic that was slightly wrong. Also, Don't use `update_notification_by_id` for notifications if they are not in `created`, `sending`, `pending`, or `sent`. It silently doesn't update them. I didn't want to do a deeper investigation into the reasons behind this terrifying state machine as part of this commit so I just changed the functions to call `dao_update_notification` manually --- app/celery/letters_pdf_tasks.py | 16 ++++--- app/letters/utils.py | 4 +- tests/app/celery/test_letters_pdf_tasks.py | 53 ++++++++++++++++++++-- 3 files changed, 61 insertions(+), 12 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index c3a47608d..2891c68db 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -27,8 +27,9 @@ from app.letters.utils import ( get_reference_from_filename, get_folder_name, upload_letter_pdf, - move_failed_pdf, ScanErrorType, + move_failed_pdf, + move_scan_to_invalid_pdf_bucket, move_error_pdf_to_scan_bucket, get_file_names_from_error_bucket ) @@ -41,8 +42,6 @@ from app.models import ( NOTIFICATION_VALIDATION_FAILED ) -from app.letters.utils import move_scan_to_invalid_pdf_bucket - @notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300) @statsd(namespace="tasks") @@ -194,8 +193,11 @@ def process_virus_scan_passed(self, filename): if not new_pdf: current_app.logger.info('Invalid precompiled pdf received {} ({})'.format(notification.id, filename)) - update_notification_status_by_id(notification.id, NOTIFICATION_VALIDATION_FAILED) - move_scan_to_invalid_pdf_bucket(scan_pdf_object) + + notification.status = NOTIFICATION_VALIDATION_FAILED + dao_update_notification(notification) + + move_scan_to_invalid_pdf_bucket(filename) scan_pdf_object.delete() return else: @@ -257,7 +259,9 @@ def _sanitise_precomiled_pdf(self, notification, precompiled_pdf): current_app.logger.exception( "RETRY FAILED: sanitise_precomiled_pdf failed for notification {}".format(notification.id), ) - update_notification_status_by_id(notification.id, NOTIFICATION_TECHNICAL_FAILURE) + + notification.status = NOTIFICATION_TECHNICAL_FAILURE + dao_update_notification(notification) raise diff --git a/app/letters/utils.py b/app/letters/utils.py index 46ff1c4c7..57395fc87 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -108,9 +108,9 @@ def move_error_pdf_to_scan_bucket(source_filename): def move_scan_to_invalid_pdf_bucket(source_filename): + scan_bucket = current_app.config['LETTERS_SCAN_BUCKET_NAME'] invalid_pdf_bucket = current_app.config['INVALID_PDF_BUCKET_NAME'] - invalid_pdf = source_filename - _move_s3_object(invalid_pdf_bucket, invalid_pdf, invalid_pdf_bucket, source_filename) + _move_s3_object(scan_bucket, source_filename, invalid_pdf_bucket, source_filename) def get_file_names_from_error_bucket(): diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index ee770cc99..690c7bb17 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -32,8 +32,11 @@ from app.models import ( NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, NOTIFICATION_VIRUS_SCAN_FAILED, + NOTIFICATION_VALIDATION_FAILED, NOTIFICATION_SENDING, - NOTIFICATION_TECHNICAL_FAILURE) + NOTIFICATION_TECHNICAL_FAILURE, + NOTIFICATION_PENDING_VIRUS_CHECK, +) from tests.conftest import set_config_values @@ -351,7 +354,7 @@ def test_process_letter_task_check_virus_scan_passed( s3 = boto3.client('s3', region_name='eu-west-1') s3.put_object(Bucket=source_bucket_name, Key=filename, Body=b'pdf_content') - sample_letter_notification.status = 'pending-virus-check' + sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK sample_letter_notification.key_type = key_type mock_s3upload = mocker.patch('app.celery.letters_pdf_tasks.s3upload') mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precomiled_pdf', return_value="success") @@ -359,22 +362,60 @@ def test_process_letter_task_check_virus_scan_passed( process_virus_scan_passed(filename) assert sample_letter_notification.status == noti_status + mock_sanitise.assert_called_once_with( + ANY, + sample_letter_notification, + b'pdf_content' + ) mock_s3upload.assert_called_once_with( bucket_name=target_bucket_name, filedata="success", file_location=destination_folder + filename, region='eu-west-1', ) + + +@freeze_time('2018-01-01 18:00') +@mock_s3 +@pytest.mark.parametrize('key_type,is_test_letter', [ + (KEY_TYPE_NORMAL, False), (KEY_TYPE_TEST, True) +]) +def test_process_letter_task_check_virus_scan_passed_when_sanitise_fails( + sample_letter_notification, mocker, key_type, is_test_letter +): + filename = 'NOTIFY.{}'.format(sample_letter_notification.reference) + source_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + target_bucket_name = current_app.config['INVALID_PDF_BUCKET_NAME'] + + conn = boto3.resource('s3', region_name='eu-west-1') + conn.create_bucket(Bucket=source_bucket_name) + conn.create_bucket(Bucket=target_bucket_name) + + s3 = boto3.client('s3', region_name='eu-west-1') + s3.put_object(Bucket=source_bucket_name, Key=filename, Body=b'pdf_content') + + sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK + sample_letter_notification.key_type = key_type + mock_move_s3 = mocker.patch('app.letters.utils._move_s3_object') + mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precomiled_pdf', return_value=None) + + process_virus_scan_passed(filename) + + assert sample_letter_notification.status == NOTIFICATION_VALIDATION_FAILED mock_sanitise.assert_called_once_with( ANY, sample_letter_notification, b'pdf_content' ) + mock_move_s3.assert_called_once_with( + source_bucket_name, filename, + target_bucket_name, filename + ) def test_process_letter_task_check_virus_scan_failed(sample_letter_notification, mocker): filename = 'NOTIFY.{}'.format(sample_letter_notification.reference) - sample_letter_notification.status = 'pending-virus-check' + sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK mock_move_failed_pdf = mocker.patch('app.celery.letters_pdf_tasks.move_failed_pdf') with pytest.raises(VirusScanError) as e: @@ -387,7 +428,7 @@ def test_process_letter_task_check_virus_scan_failed(sample_letter_notification, def test_process_letter_task_check_virus_scan_error(sample_letter_notification, mocker): filename = 'NOTIFY.{}'.format(sample_letter_notification.reference) - sample_letter_notification.status = 'pending-virus-check' + sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK mock_move_failed_pdf = mocker.patch('app.celery.letters_pdf_tasks.move_failed_pdf') with pytest.raises(VirusScanError) as e: @@ -419,6 +460,7 @@ def test_replay_letters_in_error_for_one_file(notify_api, mocker): def test_sanitise_precompiled_pdf_returns_data_from_template_preview(rmock, sample_letter_notification): + sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK rmock.post('http://localhost:9999/precompiled/sanitise', content=b'new_pdf', status_code=200) mock_celery = Mock(**{'retry.side_effect': Retry}) @@ -429,6 +471,7 @@ def test_sanitise_precompiled_pdf_returns_data_from_template_preview(rmock, samp def test_sanitise_precompiled_pdf_returns_none_on_validation_error(rmock, sample_letter_notification): + sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK rmock.post('http://localhost:9999/precompiled/sanitise', content=b'new_pdf', status_code=400) mock_celery = Mock(**{'retry.side_effect': Retry}) @@ -438,6 +481,7 @@ def test_sanitise_precompiled_pdf_returns_none_on_validation_error(rmock, sample def test_sanitise_precompiled_pdf_retries_on_http_error(rmock, sample_letter_notification): + sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK rmock.post('http://localhost:9999/precompiled/sanitise', content=b'new_pdf', status_code=500) mock_celery = Mock(**{'retry.side_effect': Retry}) @@ -449,6 +493,7 @@ def test_sanitise_precompiled_pdf_sets_notification_to_technical_failure_after_t rmock, sample_letter_notification ): + sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK rmock.post('http://localhost:9999/precompiled/sanitise', content=b'new_pdf', status_code=500) mock_celery = Mock(**{'retry.side_effect': MaxRetriesExceededError}) From 162ed0ba5fca309c4eae627388ac0f0b4be8d06c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 17 Oct 2018 15:29:39 +0100 Subject: [PATCH 08/11] remove dupes from cfg --- app/config.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/config.py b/app/config.py index 3f6b077f5..5a17d006f 100644 --- a/app/config.py +++ b/app/config.py @@ -332,7 +332,6 @@ class Development(Config): SQLALCHEMY_ECHO = False CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' - LETTERS_PDF_BUCKET_NAME = 'development-letters-pdf' TEST_LETTERS_BUCKET_NAME = 'development-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'notify.tools-ftp' LETTERS_PDF_BUCKET_NAME = 'development-letters-pdf' @@ -375,7 +374,6 @@ class Test(Development): TESTING = True CSV_UPLOAD_BUCKET_NAME = 'test-notifications-csv-upload' - LETTERS_PDF_BUCKET_NAME = 'test-letters-pdf' TEST_LETTERS_BUCKET_NAME = 'test-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'test.notify.com-ftp' LETTERS_PDF_BUCKET_NAME = 'test-letters-pdf' @@ -404,7 +402,6 @@ class Preview(Config): NOTIFY_EMAIL_DOMAIN = 'notify.works' NOTIFY_ENVIRONMENT = 'preview' CSV_UPLOAD_BUCKET_NAME = 'preview-notifications-csv-upload' - LETTERS_PDF_BUCKET_NAME = 'preview-letters-pdf' TEST_LETTERS_BUCKET_NAME = 'preview-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp' LETTERS_PDF_BUCKET_NAME = 'preview-letters-pdf' @@ -419,7 +416,6 @@ class Staging(Config): NOTIFY_EMAIL_DOMAIN = 'staging-notify.works' NOTIFY_ENVIRONMENT = 'staging' CSV_UPLOAD_BUCKET_NAME = 'staging-notify-csv-upload' - LETTERS_PDF_BUCKET_NAME = 'staging-letters-pdf' TEST_LETTERS_BUCKET_NAME = 'staging-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'staging-notify.works-ftp' LETTERS_PDF_BUCKET_NAME = 'staging-letters-pdf' @@ -436,7 +432,6 @@ class Live(Config): NOTIFY_EMAIL_DOMAIN = 'notifications.service.gov.uk' NOTIFY_ENVIRONMENT = 'live' CSV_UPLOAD_BUCKET_NAME = 'live-notifications-csv-upload' - LETTERS_PDF_BUCKET_NAME = 'production-letters-pdf' TEST_LETTERS_BUCKET_NAME = 'production-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'notifications.service.gov.uk-ftp' LETTERS_PDF_BUCKET_NAME = 'production-letters-pdf' From c63b147a2d026b89161ef818f6dec8a0ea55ee44 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 17 Oct 2018 16:09:30 +0100 Subject: [PATCH 09/11] make sure validation_failed notifications still return pdf it's useful to see them, and previously it was crashing. --- app/letters/utils.py | 29 ++++++++------ tests/app/letters/test_letter_utils.py | 55 ++++++++++++++++++++------ 2 files changed, 59 insertions(+), 25 deletions(-) diff --git a/app/letters/utils.py b/app/letters/utils.py index 57395fc87..fc84b50ac 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -6,7 +6,7 @@ from flask import current_app from notifications_utils.s3 import s3upload -from app.models import KEY_TYPE_TEST, SECOND_CLASS, RESOLVE_POSTAGE_FOR_FILE_NAME +from app.models import KEY_TYPE_TEST, SECOND_CLASS, RESOLVE_POSTAGE_FOR_FILE_NAME, NOTIFICATION_VALIDATION_FAILED from app.utils import convert_utc_to_bst @@ -48,14 +48,23 @@ def get_letter_pdf_filename(reference, crown, is_scan_letter=False, postage=SECO return upload_file_name -def get_bucket_prefix_for_notification(notification, is_test_letter=False): +def get_bucket_name_and_prefix_for_notification(notification): + is_test_letter = notification.key_type == KEY_TYPE_TEST and notification.template.is_precompiled_letter + folder = '' + if notification.status == NOTIFICATION_VALIDATION_FAILED: + bucket_name = current_app.config['INVALID_PDF_BUCKET_NAME'] + elif is_test_letter: + bucket_name = current_app.config['TEST_LETTERS_BUCKET_NAME'] + else: + bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + folder = '{}/'.format(notification.created_at.date()) + upload_file_name = PRECOMPILED_BUCKET_PREFIX.format( - folder='' if is_test_letter else - '{}/'.format(notification.created_at.date()), + folder=folder, reference=notification.reference ).upper() - return upload_file_name + return bucket_name, upload_file_name def get_reference_from_filename(filename): @@ -122,18 +131,12 @@ def get_file_names_from_error_bucket(): def get_letter_pdf(notification): - is_test_letter = notification.key_type == KEY_TYPE_TEST and notification.template.is_precompiled_letter - if is_test_letter: - bucket_name = current_app.config['TEST_LETTERS_BUCKET_NAME'] - else: - bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + bucket_name, prefix = get_bucket_name_and_prefix_for_notification(notification) s3 = boto3.resource('s3') bucket = s3.Bucket(bucket_name) - item = next(x for x in bucket.objects.filter( - Prefix=get_bucket_prefix_for_notification(notification, is_test_letter) - )) + item = next(x for x in bucket.objects.filter(Prefix=prefix)) obj = s3.Object( bucket_name=bucket_name, diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index e7068dc9f..f2077b786 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -7,33 +7,39 @@ from freezegun import freeze_time from moto import mock_s3 from app.letters.utils import ( - get_bucket_prefix_for_notification, + get_bucket_name_and_prefix_for_notification, get_letter_pdf_filename, get_letter_pdf, upload_letter_pdf, ScanErrorType, move_failed_pdf, get_folder_name ) -from app.models import KEY_TYPE_NORMAL, KEY_TYPE_TEST, PRECOMPILED_TEMPLATE_NAME +from app.models import KEY_TYPE_NORMAL, KEY_TYPE_TEST, PRECOMPILED_TEMPLATE_NAME, NOTIFICATION_VALIDATION_FAILED from tests.app.db import create_notification FROZEN_DATE_TIME = "2018-03-14 17:00:00" -@pytest.fixture() -def sample_precompiled_letter_notification_using_test_key(sample_letter_notification): +@pytest.fixture(name='sample_precompiled_letter_notification') +def _sample_precompiled_letter_notification(sample_letter_notification): sample_letter_notification.template.hidden = True sample_letter_notification.template.name = PRECOMPILED_TEMPLATE_NAME - sample_letter_notification.key_type = KEY_TYPE_TEST sample_letter_notification.reference = 'foo' with freeze_time(FROZEN_DATE_TIME): sample_letter_notification.created_at = datetime.utcnow() return sample_letter_notification -def test_get_bucket_prefix_for_notification_valid_notification(sample_notification): +@pytest.fixture(name='sample_precompiled_letter_notification_using_test_key') +def _sample_precompiled_letter_notification_using_test_key(sample_precompiled_letter_notification): + sample_precompiled_letter_notification.key_type = KEY_TYPE_TEST + return sample_precompiled_letter_notification - bucket_prefix = get_bucket_prefix_for_notification(sample_notification) +def test_get_bucket_name_and_prefix_for_notification_valid_notification(sample_notification): + + bucket, bucket_prefix = get_bucket_name_and_prefix_for_notification(sample_notification) + + assert bucket == current_app.config['LETTERS_PDF_BUCKET_NAME'] assert bucket_prefix == '{folder}/NOTIFY.{reference}'.format( folder=sample_notification.created_at.date(), reference=sample_notification.reference @@ -41,19 +47,44 @@ def test_get_bucket_prefix_for_notification_valid_notification(sample_notificati @freeze_time(FROZEN_DATE_TIME) -def test_get_bucket_prefix_for_notification_precompiled_letter_using_test_key( +def test_get_bucket_name_and_prefix_for_notification_precompiled_letter_using_test_key( sample_precompiled_letter_notification_using_test_key ): - bucket_prefix = get_bucket_prefix_for_notification( - sample_precompiled_letter_notification_using_test_key, is_test_letter=True) + bucket, bucket_prefix = get_bucket_name_and_prefix_for_notification( + sample_precompiled_letter_notification_using_test_key) + assert bucket == current_app.config['TEST_LETTERS_BUCKET_NAME'] assert bucket_prefix == 'NOTIFY.{}'.format( sample_precompiled_letter_notification_using_test_key.reference).upper() -def test_get_bucket_prefix_for_notification_invalid_notification(): +@freeze_time(FROZEN_DATE_TIME) +def test_get_bucket_name_and_prefix_for_failed_validation(sample_precompiled_letter_notification): + sample_precompiled_letter_notification.status = NOTIFICATION_VALIDATION_FAILED + bucket, bucket_prefix = get_bucket_name_and_prefix_for_notification(sample_precompiled_letter_notification) + + assert bucket == current_app.config['INVALID_PDF_BUCKET_NAME'] + assert bucket_prefix == 'NOTIFY.{}'.format( + sample_precompiled_letter_notification.reference).upper() + + +@freeze_time(FROZEN_DATE_TIME) +def test_get_bucket_name_and_prefix_for_test_noti_with_failed_validation( + sample_precompiled_letter_notification_using_test_key +): + sample_precompiled_letter_notification_using_test_key.status = NOTIFICATION_VALIDATION_FAILED + bucket, bucket_prefix = get_bucket_name_and_prefix_for_notification( + sample_precompiled_letter_notification_using_test_key + ) + + assert bucket == current_app.config['INVALID_PDF_BUCKET_NAME'] + assert bucket_prefix == 'NOTIFY.{}'.format( + sample_precompiled_letter_notification_using_test_key.reference).upper() + + +def test_get_bucket_name_and_prefix_for_notification_invalid_notification(): with pytest.raises(AttributeError): - get_bucket_prefix_for_notification(None) + get_bucket_name_and_prefix_for_notification(None) @pytest.mark.parametrize('crown_flag,expected_crown_text', [ From 3589f1dd6382ca24bb0538e03588d2608d02319a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 17 Oct 2018 16:12:57 +0100 Subject: [PATCH 10/11] remove some use of calling fixtures directly also experiment with giving the fixtures a different name, so we can be sure that they won't be called in functions. Open to change on this. --- tests/app/conftest.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index a1d71d2dd..04d77a60e 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -189,24 +189,22 @@ def sample_service( return service -@pytest.fixture(scope='function') -def sample_service_full_permissions(notify_db, notify_db_session): - return sample_service( - notify_db, - notify_db_session, - # ensure name doesn't clash with regular sample service +@pytest.fixture(scope='function', name='sample_service_full_permissions') +def _sample_service_full_permissions(notify_db_session): + service = create_service( service_name="sample service full permissions", - permissions=set(SERVICE_PERMISSION_TYPES) + service_permissions=set(SERVICE_PERMISSION_TYPES) ) - - -@pytest.fixture(scope='function') -def sample_service_custom_letter_contact_block(notify_db, notify_db_session): - service = sample_service(notify_db, notify_db_session) - create_letter_contact(service, contact_block='((contact block))') + create_inbound_number('12345', service_id=service.id) return service +@pytest.fixture(scope='function', name='sample_service_custom_letter_contact_block') +def _sample_service_custom_letter_contact_block(sample_service): + create_letter_contact(sample_service, contact_block='((contact block))') + return sample_service + + @pytest.fixture(scope='function') def sample_template( notify_db, @@ -226,7 +224,9 @@ def sample_template( if user is None: user = create_user() if service is None: - service = sample_service(notify_db, notify_db_session, permissions=permissions) + service = Service.query.filter_by(name='Sample service').first() + if not service: + service = create_service(service_permissions=permissions) if created_by is None: created_by = create_user() From e7dad9436f612e0803bc445818e3452faeee503b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 18 Oct 2018 12:19:07 +0100 Subject: [PATCH 11/11] compare service id to UUID it doesn't match a string :weary: I couldn't think of a good way to test this in a unit test, since it involves changing the service id on all of the components of a service. --- app/celery/letters_pdf_tasks.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 2891c68db..0cc6bc8f0 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -1,3 +1,4 @@ +from uuid import UUID import math from datetime import datetime @@ -187,7 +188,7 @@ def process_virus_scan_passed(self, filename): new_pdf = _sanitise_precomiled_pdf(self, notification, old_pdf) # TODO: Remove this once CYSP update their template to not cross over the margins - if notification.service_id == 'fe44178f-3b45-4625-9f85-2264a36dd9ec': # CYSP + if notification.service_id == UUID('fe44178f-3b45-4625-9f85-2264a36dd9ec'): # CYSP # Check your state pension submit letters with good addresses and notify tags, so just use their supplied pdf new_pdf = old_pdf