From a833bbf6d56030793b0ce5135135a3162af8bcb7 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 28 Dec 2017 18:18:32 +0000 Subject: [PATCH 01/14] Remove `build-codedeploy-artifact` target Also remove appspec files that are not used in the paas world. --- Makefile | 18 ++++-------------- appspec-db-migration.yml | 18 ------------------ appspec.yml | 32 -------------------------------- 3 files changed, 4 insertions(+), 64 deletions(-) delete mode 100644 appspec-db-migration.yml delete mode 100644 appspec.yml diff --git a/Makefile b/Makefile index ea6bb8c41..297bd28a2 100644 --- a/Makefile +++ b/Makefile @@ -85,19 +85,6 @@ build: dependencies generate-version-file ## Build project .PHONY: cf-build cf-build: dependencies generate-version-file ## Build project for PAAS -.PHONY: build-codedeploy-artifact -build-codedeploy-artifact: ## Build the deploy artifact for CodeDeploy - rm -rf target - mkdir -p target - zip -y -q -r -x@deploy-exclude.lst target/notifications-api.zip ./ - - rm -rf build/db-migration-codedeploy - mkdir -p build/db-migration-codedeploy - unzip target/notifications-api.zip -d build/db-migration-codedeploy - cd build/db-migration-codedeploy && \ - mv -f appspec-db-migration.yml appspec.yml && \ - zip -y -q -r -x@deploy-exclude.lst ../../target/notifications-api-db-migration.zip ./ - .PHONY: upload-codedeploy-artifact ## Upload the deploy artifact for CodeDeploy upload-codedeploy-artifact: check-env-vars $(if ${DEPLOY_BUILD_NUMBER},,$(error Must specify DEPLOY_BUILD_NUMBER)) @@ -105,7 +92,10 @@ upload-codedeploy-artifact: check-env-vars aws s3 cp --region eu-west-1 --sse AES256 target/notifications-api-db-migration.zip s3://${DNS_NAME}-codedeploy/notifications-api-db-migration-${DEPLOY_BUILD_NUMBER}.zip .PHONY: build-paas-artifact -build-paas-artifact: build-codedeploy-artifact ## Build the deploy artifact for PaaS +build-paas-artifact: ## Build the deploy artifact for PaaS + rm -rf target + mkdir -p target + zip -y -q -r -x@deploy-exclude.lst target/notifications-api.zip ./ .PHONY: upload-paas-artifact ## Upload the deploy artifact for PaaS upload-paas-artifact: diff --git a/appspec-db-migration.yml b/appspec-db-migration.yml deleted file mode 100644 index b840f4432..000000000 --- a/appspec-db-migration.yml +++ /dev/null @@ -1,18 +0,0 @@ ---- -os: linux -version: 0.0 -files: - - destination: /home/notify-app/notifications-api - source: / -hooks: - AfterInstall: - - location: scripts/aws_install_dependencies.sh - runas: root - timeout: 1000 - - location: scripts/aws_change_ownership.sh - runas: root - timeout: 300 - ApplicationStart: - - location: scripts/aws_run_db_migrations.sh - runas: root - timeout: 300 diff --git a/appspec.yml b/appspec.yml deleted file mode 100644 index 87a0f944a..000000000 --- a/appspec.yml +++ /dev/null @@ -1,32 +0,0 @@ ---- -os: linux -version: 0.0 -files: - - destination: /home/notify-app/notifications-api - source: / -hooks: - BeforeInstall: - - location: scripts/aws_clear_instance.sh - runas: root - timeout: 1000 - AfterInstall: - - location: scripts/aws_install_dependencies.sh - runas: root - timeout: 1000 - - location: scripts/aws_change_ownership.sh - runas: root - timeout: 300 - ApplicationStart: - - location: scripts/aws_start_app.sh - runas: root - timeout: 300 - - location: scripts/register_with_elb.sh - runas: ubuntu - timeout: 300 - ApplicationStop: - - location: scripts/deregister_from_elb.sh - runas: ubuntu - timeout: 300 - - location: scripts/aws_stop_app.sh - runas: root - timeout: 300 From 2b829e1f171fb765fe8c15454d2233d8d3687e0d Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 28 Dec 2017 18:21:39 +0000 Subject: [PATCH 02/14] Remove `upload-codedeploy-artifact` target --- Makefile | 6 ------ 1 file changed, 6 deletions(-) diff --git a/Makefile b/Makefile index 297bd28a2..37c2381df 100644 --- a/Makefile +++ b/Makefile @@ -85,12 +85,6 @@ build: dependencies generate-version-file ## Build project .PHONY: cf-build cf-build: dependencies generate-version-file ## Build project for PAAS -.PHONY: upload-codedeploy-artifact ## Upload the deploy artifact for CodeDeploy -upload-codedeploy-artifact: check-env-vars - $(if ${DEPLOY_BUILD_NUMBER},,$(error Must specify DEPLOY_BUILD_NUMBER)) - aws s3 cp --region eu-west-1 --sse AES256 target/notifications-api.zip s3://${DNS_NAME}-codedeploy/notifications-api-${DEPLOY_BUILD_NUMBER}.zip - aws s3 cp --region eu-west-1 --sse AES256 target/notifications-api-db-migration.zip s3://${DNS_NAME}-codedeploy/notifications-api-db-migration-${DEPLOY_BUILD_NUMBER}.zip - .PHONY: build-paas-artifact build-paas-artifact: ## Build the deploy artifact for PaaS rm -rf target From d487d57509a07b8d804fcda782fb7cb7dd5f3a64 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 28 Dec 2017 18:24:36 +0000 Subject: [PATCH 03/14] Remove targets related to AWS deployments Also remove checks for unused variables --- Makefile | 38 -------------------------------------- 1 file changed, 38 deletions(-) diff --git a/Makefile b/Makefile index 37c2381df..4d0b312fb 100644 --- a/Makefile +++ b/Makefile @@ -42,8 +42,6 @@ venv/bin/activate: check-env-vars: ## Check mandatory environment variables $(if ${DEPLOY_ENV},,$(error Must specify DEPLOY_ENV)) $(if ${DNS_NAME},,$(error Must specify DNS_NAME)) - $(if ${AWS_ACCESS_KEY_ID},,$(error Must specify AWS_ACCESS_KEY_ID)) - $(if ${AWS_SECRET_ACCESS_KEY},,$(error Must specify AWS_SECRET_ACCESS_KEY)) .PHONY: sandbox sandbox: ## Set environment to sandbox @@ -101,42 +99,6 @@ upload-paas-artifact: test: venv generate-version-file ## Run tests ./scripts/run_tests.sh -.PHONY: deploy-api -deploy-api: check-env-vars ## Trigger CodeDeploy for the api - aws deploy create-deployment --application-name notify-api --deployment-config-name CodeDeployDefault.OneAtATime --deployment-group-name notify-api --s3-location bucket=${DNS_NAME}-codedeploy,key=notifications-api-${DEPLOY_BUILD_NUMBER}.zip,bundleType=zip --region eu-west-1 - -.PHONY: deploy-api -deploy-api-db-migration: check-env-vars ## Trigger CodeDeploy for the api db migration - aws deploy create-deployment --application-name notify-api-db-migration --deployment-config-name CodeDeployDefault.OneAtATime --deployment-group-name notify-api-db-migration --s3-location bucket=${DNS_NAME}-codedeploy,key=notifications-api-db-migration-${DEPLOY_BUILD_NUMBER}.zip,bundleType=zip --region eu-west-1 - -.PHONY: deploy-admin-api -deploy-admin-api: check-env-vars ## Trigger CodeDeploy for the admin api - aws deploy create-deployment --application-name notify-admin-api --deployment-config-name CodeDeployDefault.OneAtATime --deployment-group-name notify-admin-api --s3-location bucket=${DNS_NAME}-codedeploy,key=notifications-api-${DEPLOY_BUILD_NUMBER}.zip,bundleType=zip --region eu-west-1 - -.PHONY: deploy-delivery -deploy-delivery: check-env-vars ## Trigger CodeDeploy for the delivery app - aws deploy create-deployment --application-name notify-delivery --deployment-config-name CodeDeployDefault.OneAtATime --deployment-group-name notify-delivery --s3-location bucket=${DNS_NAME}-codedeploy,key=notifications-api-${DEPLOY_BUILD_NUMBER}.zip,bundleType=zip --region eu-west-1 - -.PHONY: check-aws-vars -check-aws-vars: ## Check if AWS access keys are set - $(if ${AWS_ACCESS_KEY_ID},,$(error Must specify AWS_ACCESS_KEY_ID)) - $(if ${AWS_SECRET_ACCESS_KEY},,$(error Must specify AWS_SECRET_ACCESS_KEY)) - -.PHONY: deploy-suspend-autoscaling-processes -deploy-suspend-autoscaling-processes: check-aws-vars ## Suspend launch and terminate processes for the auto-scaling group - $(if ${CODEDEPLOY_APP_NAME},,$(error Must specify CODEDEPLOY_APP_NAME)) - aws autoscaling suspend-processes --region eu-west-1 --auto-scaling-group-name ${CODEDEPLOY_APP_NAME} --scaling-processes "Launch" "Terminate" - -.PHONY: deploy-resume-autoscaling-processes -deploy-resume-autoscaling-processes: check-aws-vars ## Resume launch and terminate processes for the auto-scaling group - $(if ${CODEDEPLOY_APP_NAME},,$(error Must specify CODEDEPLOY_APP_NAME)) - aws autoscaling resume-processes --region eu-west-1 --auto-scaling-group-name ${CODEDEPLOY_APP_NAME} --scaling-processes "Launch" "Terminate" - -.PHONY: deploy-check-autoscaling-processes -deploy-check-autoscaling-processes: check-aws-vars ## Returns with the number of instances with active autoscaling events - $(if ${CODEDEPLOY_APP_NAME},,$(error Must specify CODEDEPLOY_APP_NAME)) - @aws autoscaling describe-auto-scaling-groups --region eu-west-1 --auto-scaling-group-names ${CODEDEPLOY_APP_NAME} | jq '.AutoScalingGroups[0].Instances|map(select(.LifecycleState != "InService"))|length' - .PHONY: coverage coverage: venv ## Create coverage report . venv/bin/activate && coveralls From 4cc8bfb17469363f8da0a025dff8ac2d551d9432 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 28 Dec 2017 18:28:02 +0000 Subject: [PATCH 04/14] Remove unused DNS_NAME variable --- Makefile | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Makefile b/Makefile index 4d0b312fb..1f19939b0 100644 --- a/Makefile +++ b/Makefile @@ -41,30 +41,25 @@ venv/bin/activate: .PHONY: check-env-vars check-env-vars: ## Check mandatory environment variables $(if ${DEPLOY_ENV},,$(error Must specify DEPLOY_ENV)) - $(if ${DNS_NAME},,$(error Must specify DNS_NAME)) .PHONY: sandbox sandbox: ## Set environment to sandbox $(eval export DEPLOY_ENV=sandbox) - $(eval export DNS_NAME="cloudapps.digital") @true .PHONY: preview preview: ## Set environment to preview $(eval export DEPLOY_ENV=preview) - $(eval export DNS_NAME="notify.works") @true .PHONY: staging staging: ## Set environment to staging $(eval export DEPLOY_ENV=staging) - $(eval export DNS_NAME="staging-notify.works") @true .PHONY: production production: ## Set environment to production $(eval export DEPLOY_ENV=production) - $(eval export DNS_NAME="notifications.service.gov.uk") @true .PHONY: dependencies From e06b15aa3bf6d6b73475669997a09f97e4502674 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 28 Dec 2017 18:28:51 +0000 Subject: [PATCH 05/14] Remove unused cf-build and cf-build-with-docker We are using the non-cf versions of these commands --- Makefile | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/Makefile b/Makefile index 1f19939b0..f8f26a700 100644 --- a/Makefile +++ b/Makefile @@ -75,9 +75,6 @@ generate-version-file: ## Generates the app version file build: dependencies generate-version-file ## Build project . venv/bin/activate && PIP_ACCEL_CACHE=${PIP_ACCEL_CACHE} pip-accel install -r requirements.txt -.PHONY: cf-build -cf-build: dependencies generate-version-file ## Build project for PAAS - .PHONY: build-paas-artifact build-paas-artifact: ## Build the deploy artifact for PaaS rm -rf target @@ -122,25 +119,6 @@ build-with-docker: prepare-docker-build-image ## Build inside a Docker container ${DOCKER_BUILDER_IMAGE_NAME} \ gosu hostuser make build -.PHONY: cf-build-with-docker -cf-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} \ - -e BUILD_NUMBER=${BUILD_NUMBER} \ - -e BUILD_URL=${BUILD_URL} \ - -e http_proxy="${HTTP_PROXY}" \ - -e HTTP_PROXY="${HTTP_PROXY}" \ - -e https_proxy="${HTTPS_PROXY}" \ - -e HTTPS_PROXY="${HTTPS_PROXY}" \ - -e NO_PROXY="${NO_PROXY}" \ - ${DOCKER_BUILDER_IMAGE_NAME} \ - gosu hostuser make cf-build - .PHONY: test-with-docker test-with-docker: prepare-docker-build-image create-docker-test-db ## Run tests inside a Docker container @docker run -i${DOCKER_TTY} --rm \ From 806e880ec2c45c71bac88f4062caac79aee2fb11 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 28 Dec 2017 18:29:31 +0000 Subject: [PATCH 06/14] Fix .PHONY reference --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index f8f26a700..3b2d46d16 100644 --- a/Makefile +++ b/Makefile @@ -139,7 +139,7 @@ test-with-docker: prepare-docker-build-image create-docker-test-db ## Run tests ${DOCKER_BUILDER_IMAGE_NAME} \ gosu hostuser make test -.PHONY: test-with-docker +.PHONY: create-docker-test-db create-docker-test-db: ## Start the test database in a Docker container docker rm -f ${DOCKER_CONTAINER_PREFIX}-db 2> /dev/null || true @docker run -d \ From 2470faf04b051a50f5c4cff00ea0e61c16f8f321 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 2 Jan 2018 14:25:40 +0000 Subject: [PATCH 07/14] Remove unused scripts These were referenced by appspec and have not been used since we migrated to paas, so they can be removed. --- scripts/aws_change_ownership.sh | 8 --- scripts/aws_clear_instance.sh | 10 ---- scripts/aws_install_dependencies.sh | 8 --- scripts/aws_run_db_migrations.sh | 8 --- scripts/aws_start_app.sh | 20 ------- scripts/aws_stop_app.sh | 23 -------- scripts/deregister_from_elb.sh | 77 --------------------------- scripts/register_with_elb.sh | 78 --------------------------- scripts/stop_celery.py | 82 ----------------------------- 9 files changed, 314 deletions(-) delete mode 100755 scripts/aws_change_ownership.sh delete mode 100755 scripts/aws_clear_instance.sh delete mode 100755 scripts/aws_install_dependencies.sh delete mode 100755 scripts/aws_run_db_migrations.sh delete mode 100755 scripts/aws_start_app.sh delete mode 100755 scripts/aws_stop_app.sh delete mode 100755 scripts/deregister_from_elb.sh delete mode 100755 scripts/register_with_elb.sh delete mode 100755 scripts/stop_celery.py diff --git a/scripts/aws_change_ownership.sh b/scripts/aws_change_ownership.sh deleted file mode 100755 index ad0d438a7..000000000 --- a/scripts/aws_change_ownership.sh +++ /dev/null @@ -1,8 +0,0 @@ -#!/bin/bash - -set -eo pipefail - -echo "Chown application to be owned by notify-app" - -cd /home/notify-app/; -chown -R notify-app:govuk-notify-applications notifications-api diff --git a/scripts/aws_clear_instance.sh b/scripts/aws_clear_instance.sh deleted file mode 100755 index 9cdc14568..000000000 --- a/scripts/aws_clear_instance.sh +++ /dev/null @@ -1,10 +0,0 @@ -#!/bin/bash - -echo "Removing application and dependencies" - -if [ -d "/home/notify-app/notifications-api" ]; then - # Remove and re-create the directory - rm -rf /home/notify-app/notifications-api - mkdir -p /home/notify-app/notifications-api -fi - diff --git a/scripts/aws_install_dependencies.sh b/scripts/aws_install_dependencies.sh deleted file mode 100755 index b82881d45..000000000 --- a/scripts/aws_install_dependencies.sh +++ /dev/null @@ -1,8 +0,0 @@ -#!/bin/bash - -set -eo pipefail - -echo "Install dependencies" - -cd /home/notify-app/notifications-api; -pip3 install -r /home/notify-app/notifications-api/requirements.txt diff --git a/scripts/aws_run_db_migrations.sh b/scripts/aws_run_db_migrations.sh deleted file mode 100755 index 9667839d5..000000000 --- a/scripts/aws_run_db_migrations.sh +++ /dev/null @@ -1,8 +0,0 @@ -#!/usr/bin/env bash - -set -eo pipefail - -echo "Run database migrations" - -cd /home/notify-app/notifications-api; -python3 db.py db upgrade diff --git a/scripts/aws_start_app.sh b/scripts/aws_start_app.sh deleted file mode 100755 index 0c48d4859..000000000 --- a/scripts/aws_start_app.sh +++ /dev/null @@ -1,20 +0,0 @@ -#!/usr/bin/env bash - -set -eo pipefail - -function start -{ - service=$1 - if [ -e "/etc/init/${service}.conf" ] - then - echo "Starting ${service}" - service ${service} start - fi -} - -start "notifications-api" -start "notifications-api-celery-worker" -start "notifications-api-celery-worker-sender" -start "notifications-api-celery-worker-research" -start "notifications-api-celery-worker-db" -start "notifications-api-celery-beat" diff --git a/scripts/aws_stop_app.sh b/scripts/aws_stop_app.sh deleted file mode 100755 index 4ccc4ebc9..000000000 --- a/scripts/aws_stop_app.sh +++ /dev/null @@ -1,23 +0,0 @@ -#!/usr/bin/env bash - -set -eo pipefail - -function stop -{ - service=$1 - if [ -e "/etc/init/${service}.conf" ]; then - echo "stopping ${service}" - if service ${service} stop; then - echo "${service} stopped" - else - >&2 echo "Could not stop ${service}" - fi - fi -} - -stop "notifications-api" -stop "notifications-api-celery-beat" -stop "notifications-api-celery-worker" -stop "notifications-api-celery-worker-sender" -stop "notifications-api-celery-worker-research" -stop "notifications-api-celery-worker-db" diff --git a/scripts/deregister_from_elb.sh b/scripts/deregister_from_elb.sh deleted file mode 100755 index a539f0cac..000000000 --- a/scripts/deregister_from_elb.sh +++ /dev/null @@ -1,77 +0,0 @@ -#!/bin/bash -# -# Copyright 2014 Amazon.com, Inc. or its affiliates. All Rights Reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"). -# You may not use this file except in compliance with the License. -# A copy of the License is located at -# -# http://aws.amazon.com/apache2.0 -# -# or in the "license" file accompanying this file. This file is distributed -# on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either -# express or implied. See the License for the specific language governing -# permissions and limitations under the License. - -. $(dirname $0)/common_functions.sh - -msg "Running AWS CLI with region: $(get_instance_region)" - -# get this instance's ID -INSTANCE_ID=$(get_instance_id) -if [ $? != 0 -o -z "$INSTANCE_ID" ]; then - error_exit "Unable to get this instance's ID; cannot continue." -fi - -# Get current time -msg "Started $(basename $0) at $(/bin/date "+%F %T")" -start_sec=$(/bin/date +%s.%N) - -msg "Getting relevant load balancer" -INSTANCE_NAME=$(get_instance_name_from_tags $INSTANCE_ID) - -if [[ "$(tr [:upper:] [:lower:] <<< "${INSTANCE_NAME}")" =~ 'delivery' ]]; then - msg "NO ELBs for delivery" - exit 0 -fi - -get_elb_name_for_instance_name $INSTANCE_NAME -get_elb_list $INSTANCE_ID $ELB_NAME - -msg "Checking that user set at least one load balancer" -if test -z "$ELB_LIST"; then - error_exit "Must have at least one load balancer to deregister from" -fi - -# Loop through all LBs the user set, and attempt to deregister this instance from them. -for elb in $ELB_LIST; do - msg "Checking validity of load balancer named '$elb'" - validate_elb $INSTANCE_ID $elb - if [ $? != 0 ]; then - msg "Error validating $elb; cannot continue with this LB" - continue - fi - - msg "Deregistering $INSTANCE_ID from $elb" - deregister_instance $INSTANCE_ID $elb - - if [ $? != 0 ]; then - error_exit "Failed to deregister instance $INSTANCE_ID from ELB $elb" - fi -done - -# Wait for all Deregistrations to finish -msg "Waiting for instance to de-register from its load balancers" -for elb in $ELB_LIST; do - wait_for_state "elb" $INSTANCE_ID "OutOfService" $elb - if [ $? != 0 ]; then - error_exit "Failed waiting for $INSTANCE_ID to leave $elb" - fi -done - -msg "Finished $(basename $0) at $(/bin/date "+%F %T")" - -end_sec=$(/bin/date +%s.%N) -elapsed_seconds=$(echo "$end_sec - $start_sec" | /usr/bin/bc) - -msg "Elapsed time: $elapsed_seconds" diff --git a/scripts/register_with_elb.sh b/scripts/register_with_elb.sh deleted file mode 100755 index 7e6fb80ac..000000000 --- a/scripts/register_with_elb.sh +++ /dev/null @@ -1,78 +0,0 @@ -#!/bin/bash -# -# Copyright 2014 Amazon.com, Inc. or its affiliates. All Rights Reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"). -# You may not use this file except in compliance with the License. -# A copy of the License is located at -# -# http://aws.amazon.com/apache2.0 -# -# or in the "license" file accompanying this file. This file is distributed -# on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either -# express or implied. See the License for the specific language governing -# permissions and limitations under the License. - -. $(dirname $0)/common_functions.sh - -msg "Running AWS CLI with region: $(get_instance_region)" - -# get this instance's ID -INSTANCE_ID=$(get_instance_id) -if [ $? != 0 -o -z "$INSTANCE_ID" ]; then - error_exit "Unable to get this instance's ID; cannot continue." -fi - -# Get current time -msg "Started $(basename $0) at $(/bin/date "+%F %T")" -start_sec=$(/bin/date +%s.%N) - -msg "Getting relevant load balancer" -INSTANCE_NAME=$(get_instance_name_from_tags $INSTANCE_ID) - -if [[ "$(tr [:upper:] [:lower:] <<< "${INSTANCE_NAME}")" =~ 'delivery' ]]; then - msg "NO ELBs for delivery" - exit 0 -fi - -get_elb_name_for_instance_name $INSTANCE_NAME -ELB_LIST=$ELB_NAME -get_elb_list $INSTANCE_ID $ELB_NAME - -msg "Checking that user set at least one load balancer" -if test -z "$ELB_LIST"; then - error_exit "Must have at least one load balancer to register to" -fi - -# Loop through all LBs the user set, and attempt to register this instance to them. -for elb in $ELB_LIST; do - msg "Checking validity of load balancer named '$elb'" - validate_elb $INSTANCE_ID $elb - if [ $? != 0 ]; then - msg "Error validating $elb; cannot continue with this LB" - continue - fi - - msg "Registering $INSTANCE_ID to $elb" - register_instance $INSTANCE_ID $elb - - if [ $? != 0 ]; then - error_exit "Failed to register instance $INSTANCE_ID from ELB $elb" - fi -done - -# Wait for all Registrations to finish -msg "Waiting for instance to register to its load balancers" -for elb in $ELB_LIST; do - wait_for_state "elb" $INSTANCE_ID "InService" $elb - if [ $? != 0 ]; then - error_exit "Failed waiting for $INSTANCE_ID to return to $elb" - fi -done - -msg "Finished $(basename $0) at $(/bin/date "+%F %T")" - -end_sec=$(/bin/date +%s.%N) -elapsed_seconds=$(echo "$end_sec - $start_sec" | /usr/bin/bc) - -msg "Elapsed time: $elapsed_seconds" diff --git a/scripts/stop_celery.py b/scripts/stop_celery.py deleted file mode 100755 index 71b10ec03..000000000 --- a/scripts/stop_celery.py +++ /dev/null @@ -1,82 +0,0 @@ -#!/usr/bin/env python3 -""" - -Scipt used to stop celery in AWS environments. -This is used from upstart to issue a TERM signal to the master celery process. - -This will then allow the worker threads to stop, after completing -whatever tasks that are in flight. - -Note the script blocks for up to 15minutes, which is long enough to allow our -longest possible task to complete. If it can return quicker it will. - -Usage: - ./stop_celery.py - -Example: - ./stop_celery.py /tmp/celery.pid -""" - -import os -from docopt import docopt -import re -import subprocess -from time import sleep - - -def strip_white_space(from_this): - return re.sub(r'\s+', '', from_this) - - -def get_pid_from_file(filename): - """ - Open the file which MUST contain only the PID of the master celery process. - This is written to disk by the start celery command issued by upstart - """ - with open(filename) as f: - celery_pid = f.read() - return strip_white_space(celery_pid) - - -def issue_term_signal_to_pid(pid, celery_pid_file): - """ - Issues a TERM signal (15) to the master celery process. - - This method attempts to print out any response from this subprocess call. However this call is generally silent. - """ - print("Trying to stop ", celery_pid_file) - result = subprocess.Popen(['kill', '-15', pid], stdout=subprocess.PIPE, stderr=subprocess.PIPE) - for line in result.stdout.readlines(): - print(line.rstrip()) - for line in result.stderr.readlines(): - print(line.rstrip()) - - -def pid_still_running(pid): - """ - uses the proc filesystem to identify if the celery master pid is still around. - - Once the process stops this file no longer exists. Slim possibilty of a race condition here. - """ - return os.path.exists("/proc/" + pid) - - -if __name__ == "__main__": - arguments = docopt(__doc__) - celery_pid_file = arguments[''] - - celery_pid = get_pid_from_file(celery_pid_file) - - issue_term_signal_to_pid(celery_pid, celery_pid_file) - - """ - Blocking loop to check for the still running process. - 5 seconds between loops - 180 loops - Maximum block time of 900 seconds (15 minutes) - """ - iteration = 0 - while pid_still_running(celery_pid) and iteration < 180: - print("[", celery_pid_file, "] waited for ", iteration * 5, " secs") - sleep(5) - iteration += 1 From 545ff7dda60e83f00f907f2d53a5a7710db449ed Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 18 Dec 2017 16:14:09 +0000 Subject: [PATCH 08/14] Normalize outbound SMS number for job notifications Replaces 0 with 44 in UK SMS sender number to make it possible to respond to the SMS from an international number. --- app/models.py | 3 ++- tests/app/celery/test_tasks.py | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index 469bfff4a..6331c9076 100644 --- a/app/models.py +++ b/app/models.py @@ -14,6 +14,7 @@ from sqlalchemy import UniqueConstraint, CheckConstraint from notifications_utils.recipients import ( validate_email_address, validate_phone_number, + try_validate_and_format_phone_number, InvalidPhoneError, InvalidEmailError ) @@ -636,7 +637,7 @@ class TemplateBase(db.Model): elif self.template_type == EMAIL_TYPE: return self.service.get_default_reply_to_email_address() elif self.template_type == SMS_TYPE: - return self.service.get_default_sms_sender() + return try_validate_and_format_phone_number(self.service.get_default_sms_sender()) else: return None diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 882fc5b9f..18f1fa9e5 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1081,6 +1081,25 @@ def test_save_letter_uses_template_reply_to_text(mocker, notify_db_session): assert notification_db.reply_to_text == "Template address contact" +def test_save_sms_uses_sms_sender_reply_to_text(mocker, notify_db_session): + service = create_service_with_defined_sms_sender(sms_sender_value='07123123123') + template = create_template(service=service) + + notification = _notification_json(template, to="07700 900205") + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + + notification_id = uuid.uuid4() + save_sms( + service.id, + notification_id, + encryption.encrypt(notification), + key_type=KEY_TYPE_TEST + ) + + persisted_notification = Notification.query.one() + assert persisted_notification.reply_to_text == '447123123123' + + 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 From d5b6bd85fec7769ae8a25976b1a85f051d94ba70 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 18 Dec 2017 16:16:33 +0000 Subject: [PATCH 09/14] Normalize outbound SMS sender number in one off notifications --- app/models.py | 3 ++ app/service/send_notification.py | 2 +- .../service/test_send_one_off_notification.py | 54 +++++++++++++++++++ 3 files changed, 58 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index 6331c9076..214979611 100644 --- a/app/models.py +++ b/app/models.py @@ -367,6 +367,9 @@ class ServiceSmsSender(db.Model): created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) + def get_reply_to_text(self): + return try_validate_and_format_phone_number(self.sms_sender) + def serialize(self): return { "id": str(self.id), diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 53cbb2b6c..2b9be3de7 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -90,7 +90,7 @@ def get_reply_to_text(notification_type, sender_id, service, template): if notification_type == EMAIL_TYPE: reply_to = dao_get_reply_to_by_id(service.id, sender_id).email_address elif notification_type == SMS_TYPE: - reply_to = dao_get_service_sms_senders_by_id(service.id, sender_id).sms_sender + reply_to = dao_get_service_sms_senders_by_id(service.id, sender_id).get_reply_to_text() else: reply_to = template.get_reply_to_text() diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index e64102ec1..cbbe8f7dd 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -19,6 +19,7 @@ from tests.app.db import ( create_user, create_reply_to_email, create_letter_contact, + create_service_sms_sender, create_service, create_template ) @@ -238,9 +239,62 @@ def test_send_one_off_letter_notification_should_use_template_reply_to_text(samp research_mode=False, queue=None ) + assert notification.reply_to_text == "Edinburgh, ED1 1AA" +def test_send_one_off_sms_notification_should_use_sms_sender_reply_to_text(sample_service, celery_mock): + template = create_template(service=sample_service, template_type=SMS_TYPE) + sms_sender = create_service_sms_sender( + service=sample_service, + sms_sender='07123123123', + is_default=False + ) + + data = { + 'to': '07111111111', + 'template_id': str(template.id), + 'created_by': str(sample_service.created_by_id), + 'sender_id': str(sms_sender.id), + } + + notification_id = send_one_off_notification(service_id=sample_service.id, post_data=data) + notification = Notification.query.get(notification_id['id']) + celery_mock.assert_called_once_with( + notification=notification, + research_mode=False, + queue=None + ) + + assert notification.reply_to_text == "447123123123" + + +def test_send_one_off_sms_notification_should_use_default_service_reply_to_text(sample_service, celery_mock): + template = create_template(service=sample_service, template_type=SMS_TYPE) + sample_service.service_sms_senders[0].is_default = False + create_service_sms_sender( + service=sample_service, + sms_sender='07123123456', + is_default=True + ) + + data = { + 'to': '07111111111', + 'template_id': str(template.id), + 'created_by': str(sample_service.created_by_id), + } + + notification_id = send_one_off_notification(service_id=sample_service.id, post_data=data) + notification = Notification.query.get(notification_id['id']) + celery_mock.assert_called_once_with( + notification=notification, + research_mode=False, + queue=None + ) + + assert notification.reply_to_text == "447123123456" + + def test_send_one_off_notification_should_throw_exception_if_reply_to_id_doesnot_exist( sample_email_template ): From 7df9fdf42ae56dddda48bb348218b2b871e64da3 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 18 Dec 2017 16:17:20 +0000 Subject: [PATCH 10/14] Normalize outbound SMS sender number in notifications API Some of the tests don't set up a service sms_sender, so we need to check if it's None before trying to format the number. --- app/v2/notifications/post_notifications.py | 10 +++- .../notifications/test_post_notifications.py | 55 +++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 5466be234..bcb2b4b7e 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -2,6 +2,8 @@ import functools from flask import request, jsonify, current_app, abort +from notifications_utils.recipients import try_validate_and_format_phone_number + from app import api_user, authenticated_service from app.config import QueueNames from app.models import ( @@ -208,9 +210,13 @@ def get_reply_to_text(notification_type, form, template): elif notification_type == SMS_TYPE: service_sms_sender_id = form.get("sms_sender_id", None) - reply_to = check_service_sms_sender_id( + sms_sender_id = check_service_sms_sender_id( str(authenticated_service.id), service_sms_sender_id, notification_type - ) or template.get_reply_to_text() + ) + if sms_sender_id: + reply_to = try_validate_and_format_phone_number(sms_sender_id) + else: + reply_to = template.get_reply_to_text() elif notification_type == LETTER_TYPE: reply_to = template.get_reply_to_text() diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index cba00ff9b..df669d258 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -98,6 +98,34 @@ def test_post_sms_notification_uses_inbound_number_as_sender(client, notify_db_s mocked.assert_called_once_with([str(notification_id)], queue='send-sms-tasks') +def test_post_sms_notification_uses_inbound_number_reply_to_as_sender(client, notify_db_session, mocker): + service = create_service_with_inbound_number(inbound_number='07123123123') + + template = create_template(service=service, content="Hello (( Name))\nYour thing is due soon") + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = { + 'phone_number': '+447700900855', + 'template_id': str(template.id), + 'personalisation': {' Name': 'Jo'} + } + auth_header = create_authorization_header(service_id=service.id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True)) + assert validate(resp_json, post_sms_response) == resp_json + notifications = Notification.query.all() + assert len(notifications) == 1 + notification_id = notifications[0].id + assert resp_json['id'] == str(notification_id) + assert resp_json['content']['from_number'] == '447123123123' + assert notifications[0].reply_to_text == '447123123123' + mocked.assert_called_once_with([str(notification_id)], queue='send-sms-tasks') + + def test_post_sms_notification_returns_201_with_sms_sender_id( client, sample_template_with_placeholders, mocker ): @@ -125,6 +153,33 @@ def test_post_sms_notification_returns_201_with_sms_sender_id( mocked.assert_called_once_with([resp_json['id']], queue='send-sms-tasks') +def test_post_sms_notification_uses_sms_sender_id_reply_to( + client, sample_template_with_placeholders, mocker +): + sms_sender = create_service_sms_sender(service=sample_template_with_placeholders.service, sms_sender='07123123123') + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = { + 'phone_number': '+447700900855', + 'template_id': str(sample_template_with_placeholders.id), + 'personalisation': {' Name': 'Jo'}, + 'sms_sender_id': str(sms_sender.id) + } + auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True)) + assert validate(resp_json, post_sms_response) == resp_json + assert resp_json['content']['from_number'] == '447123123123' + notifications = Notification.query.all() + assert len(notifications) == 1 + assert notifications[0].reply_to_text == '447123123123' + mocked.assert_called_once_with([resp_json['id']], queue='send-sms-tasks') + + def test_notification_reply_to_text_is_original_value_if_sender_is_changed_after_post_notification( client, sample_template, mocker ): From a6a44987a122d194c98e2e741dc83f1ee73d2dee Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 20 Dec 2017 14:38:49 +0000 Subject: [PATCH 11/14] Allow admin to domain to use for invite links MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we’re doing user research we often: - start the task by inviting the participant to a service on Notify - have them use a prototype version of the admin app, hosted on a different domain Currently we can’t do both of these things together, because the invite emails always send people to `notifications.service.gov.uk` (because it’s the API that sends the emails, and the prototype admin app points at the production API). This commit changes the API to optionally allow an instance of the admin app to specify which domain should be used when generating invite links. --- app/invite/rest.py | 15 +++++++--- tests/app/invite/test_invite_rest.py | 43 ++++++++++++++++++++++------ 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/app/invite/rest.py b/app/invite/rest.py index ba263e1b3..c234dd5d1 100644 --- a/app/invite/rest.py +++ b/app/invite/rest.py @@ -23,7 +23,8 @@ register_errors(invite) @invite.route('', methods=['POST']) def create_invited_user(service_id): - invited_user, errors = invited_user_schema.load(request.get_json()) + request_json = request.get_json() + invited_user, errors = invited_user_schema.load(request_json) save_invited_user(invited_user) template = dao_get_template_by_id(current_app.config['INVITATION_EMAIL_TEMPLATE_ID']) @@ -37,7 +38,10 @@ def create_invited_user(service_id): personalisation={ 'user_name': invited_user.from_user.name, 'service_name': invited_user.service.name, - 'url': invited_user_url(invited_user.id) + 'url': invited_user_url( + invited_user.id, + request_json.get('invite_link_host'), + ), }, notification_type=EMAIL_TYPE, api_key_id=None, @@ -74,8 +78,11 @@ def update_invited_user(service_id, invited_user_id): return jsonify(data=invited_user_schema.dump(fetched).data), 200 -def invited_user_url(invited_user_id): +def invited_user_url(invited_user_id, invite_link_host=None): from notifications_utils.url_safe_token import generate_token token = generate_token(str(invited_user_id), current_app.config['SECRET_KEY'], current_app.config['DANGEROUS_SALT']) - return '{0}/invitation/{1}'.format(current_app.config['ADMIN_BASE_URL'], token) + if invite_link_host is None: + invite_link_host = current_app.config['ADMIN_BASE_URL'] + + return '{0}/invitation/{1}'.format(invite_link_host, token) diff --git a/tests/app/invite/test_invite_rest.py b/tests/app/invite/test_invite_rest.py index ebeeb3492..2071651a3 100644 --- a/tests/app/invite/test_invite_rest.py +++ b/tests/app/invite/test_invite_rest.py @@ -1,22 +1,41 @@ import json +import pytest import uuid from app.models import Notification, SMS_AUTH_TYPE, EMAIL_AUTH_TYPE from tests import create_authorization_header -def test_create_invited_user(admin_request, sample_service, mocker, invitation_email_template): +@pytest.mark.parametrize('extra_args, expected_start_of_invite_url', [ + ( + {}, + 'http://localhost:6012/invitation/' + ), + ( + {'invite_link_host': 'https://www.example.com'}, + 'https://www.example.com/invitation/' + ), +]) +def test_create_invited_user( + admin_request, + sample_service, + mocker, + invitation_email_template, + extra_args, + expected_start_of_invite_url, +): mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') email_address = 'invited_user@service.gov.uk' invite_from = sample_service.users[0] - data = { - 'service': str(sample_service.id), - 'email_address': email_address, - 'from_user': str(invite_from.id), - 'permissions': 'send_messages,manage_service,manage_api_keys', - 'auth_type': EMAIL_AUTH_TYPE - } + data = dict( + service=str(sample_service.id), + email_address=email_address, + from_user=str(invite_from.id), + permissions='send_messages,manage_service,manage_api_keys', + auth_type=EMAIL_AUTH_TYPE, + **extra_args + ) json_resp = admin_request.post( 'invite.create_invited_user', @@ -33,7 +52,15 @@ def test_create_invited_user(admin_request, sample_service, mocker, invitation_e assert json_resp['data']['id'] notification = Notification.query.first() + assert notification.reply_to_text == invite_from.email_address + + assert len(notification.personalisation.keys()) == 3 + assert notification.personalisation['service_name'] == 'Sample service' + assert notification.personalisation['user_name'] == 'Test User' + assert notification.personalisation['url'].startswith(expected_start_of_invite_url) + assert len(notification.personalisation['url']) > len(expected_start_of_invite_url) + mocked.assert_called_once_with([(str(notification.id))], queue="notify-internal-tasks") From 644b110a8ddcc8ab7a1dab83a16b8468e6629081 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 2 Jan 2018 17:18:01 +0000 Subject: [PATCH 12/14] Group letters into a max number of files for sending to DVLA Grouping the letters into a maximum number of files is necessary because the SQS task needs to be under a certain size. We also compress the task when sending. --- app/celery/letters_pdf_tasks.py | 8 ++++-- app/config.py | 1 + tests/app/celery/test_letters_pdf_tasks.py | 31 +++++++++++++++++++--- 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index ec08666b4..7096f281a 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -98,7 +98,8 @@ def collate_letter_pdfs_for_day(date): notify_celery.send_task( name=TaskNames.ZIP_AND_SEND_LETTER_PDFS, kwargs={'filenames_to_zip': filenames}, - queue=QueueNames.PROCESS_FTP + queue=QueueNames.PROCESS_FTP, + compression='zlib' ) @@ -112,7 +113,10 @@ def group_letters(letter_pdfs): list_of_files = [] for letter in letter_pdfs: if letter['Key'].lower().endswith('.pdf'): - if running_filesize + letter['Size'] > current_app.config['MAX_LETTER_PDF_ZIP_FILESIZE']: + if ( + running_filesize + letter['Size'] > current_app.config['MAX_LETTER_PDF_ZIP_FILESIZE'] or + len(list_of_files) >= current_app.config['MAX_LETTER_PDF_COUNT_PER_ZIP'] + ): yield list_of_files running_filesize = 0 list_of_files = [] diff --git a/app/config.py b/app/config.py index e42a08c3b..ba24b4a5a 100644 --- a/app/config.py +++ b/app/config.py @@ -129,6 +129,7 @@ class Config(object): MAX_VERIFY_CODE_COUNT = 10 MAX_LETTER_PDF_ZIP_FILESIZE = 500 * 1024 * 1024 # 500mb + MAX_LETTER_PDF_COUNT_PER_ZIP = 5000 CHECK_PROXY_HEADER = False diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 78a58306c..e387f6b27 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -155,16 +155,18 @@ def test_collate_letter_pdfs_for_day(notify_api, mocker): assert mock_celery.call_args_list[0] == call( name='zip-and-send-letter-pdfs', kwargs={'filenames_to_zip': ['A.PDF', 'B.pDf']}, - queue='process-ftp-tasks' + queue='process-ftp-tasks', + compression='zlib' ) assert mock_celery.call_args_list[1] == call( name='zip-and-send-letter-pdfs', kwargs={'filenames_to_zip': ['C.pdf']}, - queue='process-ftp-tasks' + queue='process-ftp-tasks', + compression='zlib' ) -def test_group_letters(notify_api): +def test_group_letters_splits_on_file_size(notify_api): letters = [ # ends under max but next one is too big {'Key': 'A.pdf', 'Size': 1}, {'Key': 'B.pdf', 'Size': 2}, @@ -190,6 +192,29 @@ def test_group_letters(notify_api): assert next(x, None) is None +def test_group_letters_splits_on_file_count(notify_api): + letters = [ + {'Key': 'A.pdf', 'Size': 1}, + {'Key': 'B.pdf', 'Size': 2}, + {'Key': 'C.pdf', 'Size': 3}, + {'Key': 'D.pdf', 'Size': 1}, + {'Key': 'E.pdf', 'Size': 1}, + {'Key': 'F.pdf', 'Size': 5}, + {'Key': 'G.pdf', 'Size': 6}, + {'Key': 'H.pdf', 'Size': 1}, + {'Key': 'I.pdf', 'Size': 1}, + ] + + with set_config_values(notify_api, {'MAX_LETTER_PDF_COUNT_PER_ZIP': 3}): + x = group_letters(letters) + + assert next(x) == [{'Key': 'A.pdf', 'Size': 1}, {'Key': 'B.pdf', 'Size': 2}, {'Key': 'C.pdf', 'Size': 3}] + assert next(x) == [{'Key': 'D.pdf', 'Size': 1}, {'Key': 'E.pdf', 'Size': 1}, {'Key': 'F.pdf', 'Size': 5}] + assert next(x) == [{'Key': 'G.pdf', 'Size': 6}, {'Key': 'H.pdf', 'Size': 1}, {'Key': 'I.pdf', 'Size': 1}] + # make sure iterator is exhausted + assert next(x, None) is None + + def test_group_letters_ignores_non_pdfs(notify_api): letters = [{'Key': 'A.zip'}] assert list(group_letters(letters)) == [] From 440012288df76cbad47b4c63a5bfef88883187e7 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 3 Jan 2018 10:42:37 +0000 Subject: [PATCH 13/14] Add test for max file size and file count when grouping letters --- tests/app/celery/test_letters_pdf_tasks.py | 34 ++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index e387f6b27..5c2532934 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -215,6 +215,40 @@ def test_group_letters_splits_on_file_count(notify_api): assert next(x, None) is None +def test_group_letters_splits_on_file_size_and_file_count(notify_api): + letters = [ + # ends under max file size but next file is too big + {'Key': 'A.pdf', 'Size': 1}, + {'Key': 'B.pdf', 'Size': 2}, + # ends on exactly max number of files and file size + {'Key': 'C.pdf', 'Size': 3}, + {'Key': 'D.pdf', 'Size': 1}, + {'Key': 'E.pdf', 'Size': 1}, + # exactly max file size goes in next file + {'Key': 'F.pdf', 'Size': 5}, + # file size is within max but number of files reaches limit + {'Key': 'G.pdf', 'Size': 1}, + {'Key': 'H.pdf', 'Size': 1}, + {'Key': 'I.pdf', 'Size': 1}, + # whatever's left goes in last list + {'Key': 'J.pdf', 'Size': 1}, + ] + + with set_config_values(notify_api, { + 'MAX_LETTER_PDF_ZIP_FILESIZE': 5, + 'MAX_LETTER_PDF_COUNT_PER_ZIP': 3 + }): + x = group_letters(letters) + + assert next(x) == [{'Key': 'A.pdf', 'Size': 1}, {'Key': 'B.pdf', 'Size': 2}] + assert next(x) == [{'Key': 'C.pdf', 'Size': 3}, {'Key': 'D.pdf', 'Size': 1}, {'Key': 'E.pdf', 'Size': 1}] + assert next(x) == [{'Key': 'F.pdf', 'Size': 5}] + assert next(x) == [{'Key': 'G.pdf', 'Size': 1}, {'Key': 'H.pdf', 'Size': 1}, {'Key': 'I.pdf', 'Size': 1}] + assert next(x) == [{'Key': 'J.pdf', 'Size': 1}] + # make sure iterator is exhausted + assert next(x, None) is None + + def test_group_letters_ignores_non_pdfs(notify_api): letters = [{'Key': 'A.zip'}] assert list(group_letters(letters)) == [] From 83711a9a0d022d470a9cb63b05f803f22aacd955 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 18 Dec 2017 17:00:45 +0000 Subject: [PATCH 14/14] Remove the delivery receipt callback for letters This has been removed because the services only see one status. Pivotal story: https://www.pivotaltracker.com/story/show/153674962 --- app/celery/tasks.py | 15 --------------- tests/app/celery/test_ftp_update_tasks.py | 13 ------------- 2 files changed, 28 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 7b5717b66..57a7c5bc8 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -31,7 +31,6 @@ 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 from app.dao.jobs_dao import ( @@ -46,13 +45,11 @@ 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, ) from app.dao.provider_details_dao import get_current_provider from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count from app.dao.templates_dao import dao_get_template_by_id -from app.dao.service_callback_api_dao import get_service_callback_api_for_service from app.models import ( DVLA_RESPONSE_STATUS_SENT, EMAIL_TYPE, @@ -416,12 +413,6 @@ def update_letter_notifications_to_error(self, notification_references): ) current_app.logger.info("Updated {} letter notifications to technical-failure".format(updated_count)) - notifications = dao_get_notifications_by_references(references=notification_references) - # queue callback task only if the service_callback_api exists - service_callback_api = get_service_callback_api_for_service(service_id=notifications[0].service_id) - if service_callback_api: - for notification in notifications: - send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.CALLBACKS) def create_dvla_file_contents_for_job(job_id): @@ -503,12 +494,6 @@ def update_letter_notifications_statuses(self, filename): current_app.logger.info( 'DVLA file: {filename}, notification updated to {status}: {reference}'.format( filename=filename, status=status, reference=str(update.reference))) - notifications = dao_get_notifications_by_references(references=[update.reference]) - # queue callback task only if the service_callback_api exists - service_callback_api = get_service_callback_api_for_service(service_id=notifications[0].service_id) - if service_callback_api: - for notification in notifications: - send_delivery_status_to_service.apply_async([str(notification.id)], queue=QueueNames.CALLBACKS) def process_updates_from_file(response_file): diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index cf3e90ade..880189297 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -23,7 +23,6 @@ from app.celery.tasks import ( from tests.app.db import create_notification, create_service_callback_api from tests.conftest import set_config -from unittest.mock import call def test_update_job_to_sent_to_dvla(sample_letter_template, sample_letter_job): @@ -103,10 +102,6 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp sent_letter.reference, failed_letter.reference) mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) - send_mock = mocker.patch( - 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' - ) - update_letter_notifications_statuses(filename='foo.txt') assert sent_letter.status == NOTIFICATION_DELIVERED @@ -116,10 +111,6 @@ def test_update_letter_notifications_statuses_persisted(notify_api, mocker, samp assert failed_letter.billable_units == 2 assert failed_letter.updated_at - calls = [call([str(failed_letter.id)], queue="service-callbacks"), - call([str(sent_letter.id)], queue="service-callbacks")] - send_mock.assert_has_calls(calls, any_order=True) - def test_update_letter_notifications_does_not_call_send_callback_if_no_db_entry(notify_api, mocker, sample_letter_template): @@ -159,9 +150,6 @@ def test_update_letter_notifications_to_error_updates_based_on_notification_refe sample_letter_template, mocker ): - send_mock = mocker.patch( - 'app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async' - ) first = create_notification(sample_letter_template, reference='first ref') second = create_notification(sample_letter_template, reference='second ref') create_service_callback_api(service=sample_letter_template.service, url="https://original_url.com") @@ -174,4 +162,3 @@ def test_update_letter_notifications_to_error_updates_based_on_notification_refe assert first.sent_at is None assert first.updated_at == dt assert second.status == NOTIFICATION_CREATED - assert send_mock.called