From 5a9d5cd065321b407724e4b510fd13a1f6c86cc0 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 2 Jan 2018 12:29:50 +0000 Subject: [PATCH 01/23] Remove unused targets and variables from Makefile Remove `cf-build` and `cf-build-with-docker` as they are not being used Remove `build-codedeploy-artifact` in favor of `build-paas-artifact` Remove `upload-codedeploy-artifact` in favor of `upload-paas-artifact` Remove `deploy`, `check-aws-vars`, `deploy-suspend-autoscaling-processes`, `deploy-resume-autoscaling-processes`, `deploy-check-autoscaling-processes` as they are remains of the pre-paas era. Consequently some variables became obsolete, namely: `CODEDEPLOY_PREFIX` `CODEDEPLOY_APP_NAME`, `DNS_NAME`, `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY` and they are removed. --- Makefile | 53 +++-------------------------------------------------- 1 file changed, 3 insertions(+), 50 deletions(-) diff --git a/Makefile b/Makefile index e255be409..d1536595b 100644 --- a/Makefile +++ b/Makefile @@ -19,9 +19,6 @@ BUILD_URL ?= DOCKER_CONTAINER_PREFIX = ${USER}-${BUILD_TAG} -CODEDEPLOY_PREFIX ?= notifications-admin -CODEDEPLOY_APP_NAME ?= notify-admin - CF_API ?= api.cloud.service.gov.uk CF_ORG ?= govuk-notify CF_SPACE ?= ${DEPLOY_ENV} @@ -42,32 +39,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)) - $(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 $(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 @@ -87,55 +77,22 @@ build: dependencies generate-version-file ## Build project npm run build . 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 - npm run build - -.PHONY: build-codedeploy-artifact -build-codedeploy-artifact: ## Build the deploy artifact for CodeDeploy +.PHONY: build-paas-artifact +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-admin.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)) - aws s3 cp --region eu-west-1 --sse AES256 target/notifications-admin.zip s3://${DNS_NAME}-codedeploy/${CODEDEPLOY_PREFIX}-${DEPLOY_BUILD_NUMBER}.zip - -.PHONY: build-paas-artifact -build-paas-artifact: build-codedeploy-artifact ## Build the deploy artifact for PaaS - .PHONY: upload-paas-artifact ## Upload the deploy artifact for PaaS upload-paas-artifact: $(if ${DEPLOY_BUILD_NUMBER},,$(error Must specify DEPLOY_BUILD_NUMBER)) $(if ${JENKINS_S3_BUCKET},,$(error Must specify JENKINS_S3_BUCKET)) - aws s3 cp --region eu-west-1 --sse AES256 target/notifications-admin.zip s3://${JENKINS_S3_BUCKET}/build/${CODEDEPLOY_PREFIX}/${DEPLOY_BUILD_NUMBER}.zip + aws s3 cp --region eu-west-1 --sse AES256 target/notifications-admin.zip s3://${JENKINS_S3_BUCKET}/build/notifications-admin/${DEPLOY_BUILD_NUMBER}.zip .PHONY: test test: venv ## Run tests ./scripts/run_tests.sh -.PHONY: deploy -deploy: check-env-vars ## Upload deploy artifacts to S3 and trigger CodeDeploy - aws deploy create-deployment --application-name ${CODEDEPLOY_APP_NAME} --deployment-config-name CodeDeployDefault.OneAtATime --deployment-group-name ${CODEDEPLOY_APP_NAME} --s3-location bucket=${DNS_NAME}-codedeploy,key=${CODEDEPLOY_PREFIX}-${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 - 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 - 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 - @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 @@ -180,10 +137,6 @@ endef build-with-docker: prepare-docker-build-image ## Build inside a Docker container $(call run_docker_container,build,gosu hostuser make build) -.PHONY: cf-build-with-docker -cf-build-with-docker: prepare-docker-build-image ## Build inside a Docker container - $(call run_docker_container,build,gosu hostuser make cf-build) - .PHONY: test-with-docker test-with-docker: prepare-docker-build-image ## Run tests inside a Docker container $(call run_docker_container,test,gosu hostuser make test) From d8dabcfbbf9d6a6717c9fde2a698698af1f6af13 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 2 Jan 2018 14:24:26 +0000 Subject: [PATCH 02/23] Remove appspec and related files These have not been used since the migration to paas, so they can be removed. --- appspec.yml | 39 --- scripts/aws_change_ownership.sh | 5 - scripts/aws_clear_instance.sh | 10 - scripts/aws_install_dependencies.sh | 5 - scripts/aws_start_app.sh | 4 - scripts/aws_stop_app.sh | 15 -- scripts/common_functions.sh | 384 ---------------------------- scripts/deregister_from_elb.sh | 71 ----- scripts/register_with_elb.sh | 72 ------ 9 files changed, 605 deletions(-) delete mode 100644 appspec.yml 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_start_app.sh delete mode 100755 scripts/aws_stop_app.sh delete mode 100644 scripts/common_functions.sh delete mode 100755 scripts/deregister_from_elb.sh delete mode 100755 scripts/register_with_elb.sh diff --git a/appspec.yml b/appspec.yml deleted file mode 100644 index 0a55dcfa1..000000000 --- a/appspec.yml +++ /dev/null @@ -1,39 +0,0 @@ ---- -files: - - - destination: /home/notify-app/notifications-admin - source: / -hooks: - BeforeInstall: - - location: scripts/aws_clear_instance.sh - runas: root - timeout: 1000 - AfterInstall: - - - location: scripts/aws_install_dependencies.sh - runas: root - timeout: 300 - - - 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 -os: linux -version: 0.0 diff --git a/scripts/aws_change_ownership.sh b/scripts/aws_change_ownership.sh deleted file mode 100755 index e800956d3..000000000 --- a/scripts/aws_change_ownership.sh +++ /dev/null @@ -1,5 +0,0 @@ -#!/bin/bash - -echo "Chown application to be owned by ubuntu" -cd /home/notify-app/; -chown -R notify-app:govuk-notify-applications notifications-admin \ No newline at end of file diff --git a/scripts/aws_clear_instance.sh b/scripts/aws_clear_instance.sh deleted file mode 100755 index 1b159ddb3..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-admin" ]; then - # Remove and re-create the directory - rm -rf /home/notify-app/notifications-admin - mkdir -p /home/notify-app/notifications-admin -fi - diff --git a/scripts/aws_install_dependencies.sh b/scripts/aws_install_dependencies.sh deleted file mode 100755 index ccf5a6578..000000000 --- a/scripts/aws_install_dependencies.sh +++ /dev/null @@ -1,5 +0,0 @@ -#!/bin/bash - -echo "Install dependencies" -cd /home/notify-app/notifications-admin; -pip3 install --find-links=vendor -r /home/notify-app/notifications-admin/requirements.txt diff --git a/scripts/aws_start_app.sh b/scripts/aws_start_app.sh deleted file mode 100755 index b4082672a..000000000 --- a/scripts/aws_start_app.sh +++ /dev/null @@ -1,4 +0,0 @@ -#!/bin/bash - -echo "Starting application" -sudo service notifications-admin start \ No newline at end of file diff --git a/scripts/aws_stop_app.sh b/scripts/aws_stop_app.sh deleted file mode 100755 index 2b90414a1..000000000 --- a/scripts/aws_stop_app.sh +++ /dev/null @@ -1,15 +0,0 @@ -#!/bin/bash - - -function error_exit -{ - echo "$1" 1>&2 - exit 0 -} - -echo "Stopping application" -if sudo service notifications-admin stop; then - exit 0 -else - error_exit "Could not stop application" -fi \ No newline at end of file diff --git a/scripts/common_functions.sh b/scripts/common_functions.sh deleted file mode 100644 index d9fa96b3b..000000000 --- a/scripts/common_functions.sh +++ /dev/null @@ -1,384 +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. - -# ELB_LIST defines which Elastic Load Balancers this instance should be part of. -# The elements in ELB_LIST should be seperated by space. -ELB_LIST="" - -# Under normal circumstances, you shouldn't need to change anything below this line. -# ----------------------------------------------------------------------------- - -export PATH="$PATH:/usr/bin:/usr/local/bin" - -# If true, all messages will be printed. If false, only fatal errors are printed. -DEBUG=true - -# Number of times to check for a resouce to be in the desired state. -WAITER_ATTEMPTS=60 - -# Number of seconds to wait between attempts for resource to be in a state. -WAITER_INTERVAL=1 - -# AutoScaling Standby features at minimum require this version to work. -MIN_CLI_VERSION='1.3.25' - -# Usage: get_instance_region -# -# Writes to STDOUT the AWS region as known by the local instance. -get_instance_region() { - if [ -z "$AWS_REGION" ]; then - AWS_REGION=$(curl -s http://169.254.169.254/latest/dynamic/instance-identity/document \ - | grep -i region \ - | awk -F\" '{print $4}') - fi - - echo $AWS_REGION -} - -AWS_CLI="aws --region $(get_instance_region)" - - -# Usage: get_instance_state_asg -# -# Gets the state of the given as known by the AutoScaling group it's a part of. -# Health is printed to STDOUT and the function returns 0. Otherwise, no output and return is -# non-zero. -get_instance_state_asg() { - local instance_id=$1 - - local state=$($AWS_CLI autoscaling describe-auto-scaling-instances \ - --instance-ids $instance_id \ - --query "AutoScalingInstances[?InstanceId == \`$instance_id\`].LifecycleState | [0]" \ - --output text) - if [ $? != 0 ]; then - return 1 - else - echo $state - return 0 - fi -} - -reset_waiter_timeout() { - local elb=$1 - local state_name=$2 - - if [ "$state_name" == "InService" ]; then - - # Wait for a health check to succeed - local timeout=$($AWS_CLI elb describe-load-balancers \ - --load-balancer-name $elb \ - --query 'LoadBalancerDescriptions[0].HealthCheck.Timeout') - - elif [ "$state_name" == "OutOfService" ]; then - - # If connection draining is enabled, wait for connections to drain - local draining_values=$($AWS_CLI elb describe-load-balancer-attributes \ - --load-balancer-name $elb \ - --query 'LoadBalancerAttributes.ConnectionDraining.[Enabled,Timeout]' \ - --output text) - local draining_enabled=$(echo $draining_values | awk '{print $1}') - local timeout=$(echo $draining_values | awk '{print $2}') - - if [ "$draining_enabled" != "True" ]; then - timeout=0 - fi - - else - msg "Unknown state name, '$state_name'"; - return 1; - fi - - # Base register/deregister action may take up to about 30 seconds - timeout=$((timeout + 60)) - - WAITER_ATTEMPTS=$((timeout / WAITER_INTERVAL)) -} - -# Usage: wait_for_state [ELB name] -# -# Waits for the state of to be in as seen by . Returns 0 if -# it successfully made it to that state; non-zero if not. By default, checks $WAITER_ATTEMPTS -# times, every $WAITER_INTERVAL seconds. If giving an [ELB name] to check under, these are reset -# to that ELB's timeout values. -wait_for_state() { - local service=$1 - local instance_id=$2 - local state_name=$3 - local elb=$4 - - local instance_state_cmd - if [ "$service" == "elb" ]; then - instance_state_cmd="get_instance_health_elb $instance_id $elb" - reset_waiter_timeout $elb $state_name - if [ $? != 0 ]; then - error_exit "Failed resetting waiter timeout for $elb" - fi - elif [ "$service" == "autoscaling" ]; then - instance_state_cmd="get_instance_state_asg $instance_id" - else - msg "Cannot wait for instance state; unknown service type, '$service'" - return 1 - fi - - msg "Checking $WAITER_ATTEMPTS times, every $WAITER_INTERVAL seconds, for instance $instance_id to be in state $state_name" - - local instance_state=$($instance_state_cmd) - local count=1 - - msg "Instance is currently in state: $instance_state" - while [ "$instance_state" != "$state_name" ]; do - if [ $count -ge $WAITER_ATTEMPTS ]; then - local timeout=$(($WAITER_ATTEMPTS * $WAITER_INTERVAL)) - msg "Instance failed to reach state, $state_name within $timeout seconds" - return 1 - fi - - sleep $WAITER_INTERVAL - - instance_state=$($instance_state_cmd) - count=$(($count + 1)) - msg "Instance is currently in state: $instance_state" - done - - return 0 -} - -# Usage: get_instance_health_elb -# -# Gets the health of the given as known by . If it's a valid health -# status (one of InService|OutOfService|Unknown), then the health is printed to STDOUT and the -# function returns 0. Otherwise, no output and return is non-zero. -get_instance_health_elb() { - local instance_id=$1 - local elb_name=$2 - - msg "Checking status of instance '$instance_id' in load balancer '$elb_name'" - - # If describe-instance-health for this instance returns an error, then it's not part of - # this ELB. But, if the call was successful let's still double check that the status is - # valid. - local instance_status=$($AWS_CLI elb describe-instance-health \ - --load-balancer-name $elb_name \ - --instances $instance_id \ - --query 'InstanceStates[].State' \ - --output text 2>/dev/null) - - if [ $? == 0 ]; then - case "$instance_status" in - InService|OutOfService|Unknown) - echo -n $instance_status - return 0 - ;; - *) - msg "Instance '$instance_id' not part of ELB '$elb_name'" - return 1 - esac - fi -} - -# Usage: validate_elb -# -# Validates that the Elastic Load Balancer with name exists, is describable, and -# contains as one of its instances. -# -# If any of these checks are false, the function returns non-zero. -validate_elb() { - local instance_id=$1 - local elb_name=$2 - - # Get the list of active instances for this LB. - local elb_instances=$($AWS_CLI elb describe-load-balancers \ - --load-balancer-name $elb_name \ - --query 'LoadBalancerDescriptions[*].Instances[*].InstanceId' \ - --output text) - if [ $? != 0 ]; then - msg "Couldn't describe ELB instance named '$elb_name'" - return 1 - fi - - msg "Checking health of '$instance_id' as known by ELB '$elb_name'" - local instance_health=$(get_instance_health_elb $instance_id $elb_name) - if [ $? != 0 ]; then - return 1 - fi - - return 0 -} - -# Usage: get_elb_list -# -# Ensures that this instance is related to the named ELB. After execution, the variable -# "ELB_LIST" will contain the list of load balancers for the given instance. -# -# If the given instance ID isn't found registered to any ELBs, the function returns non-zero -get_elb_list() { - local instance_id=$1 - local required_elb=$2 - local elb_list="" - - msg "Looking up from ELB list" - local all_balancers=$($AWS_CLI elb describe-load-balancers \ - --query LoadBalancerDescriptions[*].LoadBalancerName \ - --output text | sed -e $'s/\t/ /g') - - if [[ $all_balancers =~ $required_elb ]] - then - local instance_health - instance_health=$(get_instance_health_elb $instance_id $required_elb) - if [ $? == 0 ]; then - elb_list="$elb_list $required_elb" - fi - fi - - if [ -z "$elb_list" ]; then - return 1 - else - msg "Got load balancer list of: $elb_list" - ELB_LIST=$elb_list - return 0 - fi -} - -# Usage: deregister_instance -# -# Deregisters from . -deregister_instance() { - local instance_id=$1 - local elb_name=$2 - - $AWS_CLI elb deregister-instances-from-load-balancer \ - --load-balancer-name $elb_name \ - --instances $instance_id 1> /dev/null - - return $? -} - -# Usage: register_instance -# -# Registers to . -register_instance() { - local instance_id=$1 - local elb_name=$2 - - $AWS_CLI elb register-instances-with-load-balancer \ - --load-balancer-name $elb_name \ - --instances $instance_id 1> /dev/null - - return $? -} - -# Usage: check_cli_version [version-to-check] [desired version] -# -# Without any arguments, checks that the installed version of the AWS CLI is at least at version -# $MIN_CLI_VERSION. Returns non-zero if the version is not high enough. -check_cli_version() { - if [ -z $1 ]; then - version=$($AWS_CLI --version 2>&1 | cut -f1 -d' ' | cut -f2 -d/) - else - version=$1 - fi - - if [ -z "$2" ]; then - min_version=$MIN_CLI_VERSION - else - min_version=$2 - fi - - x=$(echo $version | cut -f1 -d.) - y=$(echo $version | cut -f2 -d.) - z=$(echo $version | cut -f3 -d.) - - min_x=$(echo $min_version | cut -f1 -d.) - min_y=$(echo $min_version | cut -f2 -d.) - min_z=$(echo $min_version | cut -f3 -d.) - - msg "Checking minimum required CLI version (${min_version}) against installed version ($version)" - - if [ $x -lt $min_x ]; then - return 1 - elif [ $y -lt $min_y ]; then - return 1 - elif [ $y -gt $min_y ]; then - return 0 - elif [ $z -ge $min_z ]; then - return 0 - else - return 1 - fi -} - -# Usage: msg -# -# Writes to STDERR only if $DEBUG is true, otherwise has no effect. -msg() { - local message=$1 - $DEBUG && echo $message 1>&2 -} - -# Usage: error_exit -# -# Writes to STDERR as a "fatal" and immediately exits the currently running script. -error_exit() { - local message=$1 - - echo "[FATAL] $message" 1>&2 - exit 1 -} - -# Usage: get_instance_id -# -# Writes to STDOUT the EC2 instance ID for the local instance. Returns non-zero if the local -# instance metadata URL is inaccessible. -get_instance_id() { - curl -s http://169.254.169.254/latest/meta-data/instance-id - return $? -} - -# Usage: get_instance_name_from_tags -# -# Looks up tags for the given instance, extracting the 'name' -# returns or error_exit -get_instance_name_from_tags() { - local instance_id=$1 - - local instance_name=$($AWS_CLI ec2 describe-tags \ - --filters "Name=resource-id,Values=${instance_id}" \ - --query Tags[0].Value \ - --output text) - if [ $? != 0 ]; then - error_exit "Couldn't get instance name for '$instance_id'" - fi - echo $instance_name - return $? -} - -ELB_NAME="" - -get_elb_name_for_instance_name() { - local instance_name=$1 - - declare -A elb_to_instance_mapping - - elb_to_instance_mapping['notify-admin']='notify-admin' - - elb_to_instance_mapping['notify-admin-ags']='notify-admin-ags' - - local elb_name=${elb_to_instance_mapping[${instance_name}]} - if [ -z $elb_name ]; then - msg "No ELB for instance ${instance_name}" - else - ELB_NAME=$elb_name - fi -} diff --git a/scripts/deregister_from_elb.sh b/scripts/deregister_from_elb.sh deleted file mode 100755 index 62bab1612..000000000 --- a/scripts/deregister_from_elb.sh +++ /dev/null @@ -1,71 +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) -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 7cc78006c..000000000 --- a/scripts/register_with_elb.sh +++ /dev/null @@ -1,72 +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) -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" From d9f6554956f543479e42eb00cfeb4474c5f3062c Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Wed, 3 Jan 2018 17:17:26 +0000 Subject: [PATCH 03/23] Add make targets to enable and disable failwhale Also add a couple of convenience targets and bring back the DNS_NAME variable. --- Makefile | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index d1536595b..b45f3fbd0 100644 --- a/Makefile +++ b/Makefile @@ -39,25 +39,30 @@ 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 @@ -173,9 +178,8 @@ cf-deploy: ## Deploys the app to Cloud Foundry cf delete -f notify-admin-rollback .PHONY: cf-deploy-prototype -cf-deploy-prototype: ## Deploys the app to Cloud Foundry +cf-deploy-prototype: cf-target ## Deploys the app to Cloud Foundry $(if ${CF_SPACE},,$(error Must specify CF_SPACE)) - cf target -s ${CF_SPACE} cf push -f manifest-prototype-${CF_SPACE}.yml .PHONY: cf-rollback @@ -188,3 +192,23 @@ cf-rollback: ## Rollbacks the app to the previous release .PHONY: cf-push cf-push: cf push -f manifest-${CF_SPACE}.yml + +.PHONY: cf-target +cf-target: check-env-vars + @cf target -o ${CF_ORG} -s ${CF_SPACE} + +.PHONY: cf-failwhale-deployed +cf-failwhale-deployed: + @cf app notify-admin-failwhale --guid || (echo "notify-admin-failwhale is not deployed on ${CF_SPACE}" && exit 1) + +.PHONY: enable-failwhale +enable-failwhale: cf-target cf-failwhale-deployed ## Enable the failwhale app and disable admin + @cf map-route notify-admin-failwhale ${DNS_NAME} --hostname www + @cf unmap-route notify-admin ${DNS_NAME} --hostname www + @echo "Failwhale is enabled" + +.PHONY: disable-failwhale +disable-failwhale: cf-target cf-failwhale-deployed ## Disable the failwhale app and enable admin + @cf map-route notify-admin ${DNS_NAME} --hostname www + @cf unmap-route notify-admin-failwhale ${DNS_NAME} --hostname www + @echo "Failwhale is disabled" From 85c7ecfe647e55e9a1fd1df60d928af5845c8f58 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Thu, 4 Jan 2018 11:02:18 +0000 Subject: [PATCH 04/23] Update docs about how to enable and disable failwhale --- paas-failwhale/README.md | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/paas-failwhale/README.md b/paas-failwhale/README.md index 25e10f6f4..540e812b5 100644 --- a/paas-failwhale/README.md +++ b/paas-failwhale/README.md @@ -10,21 +10,17 @@ It should already be deployed, but if not you can deploy it by running cf push notify-admin-failwhale -To direct traffic to it you need to update the routes by running +To enable it you need to run - cf map-route notify-admin-failwhale [ENVIRONMENT-URL] --hostname www - cf unmap-route notify-admin [ENVIRONMENT-URL] --hostname www + make enable-failwhale -To remove admin failwhale: +and to disable it - cf map-route notify-admin [ENVIRONMENT-URL] --hostname www - cf unmap-route notify-admin-failwhale [ENVIRONMENT-URL] --hostname www + make disable-failwhale -Where [ENVIRONMENT-URL] would be one of: +Where is any of -- notify.works for preview -- notify-staging.works for staging -- notifications.service.gov.uk for production - -**Make sure you are on the correct environment!** +- preview +- staging +- production From ee64d9bfee54fc900524dfe7a1bb6dc1255f8a88 Mon Sep 17 00:00:00 2001 From: Sakis Date: Thu, 4 Jan 2018 11:02:51 +0000 Subject: [PATCH 05/23] Use backticks around to make it render --- paas-failwhale/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/paas-failwhale/README.md b/paas-failwhale/README.md index 540e812b5..95e0ea107 100644 --- a/paas-failwhale/README.md +++ b/paas-failwhale/README.md @@ -19,7 +19,7 @@ and to disable it make disable-failwhale -Where is any of +Where `` is any of - preview - staging From 1e7b330bd9e660c6f117640a557ba71c6f9609a1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 4 Jan 2018 15:36:22 +0000 Subject: [PATCH 06/23] Add British Museum to whitelist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > BM is an executive non-departmental public body, sponsored by the > Department for Digital, Culture, Media & Sport. > > British Museum has a separate website (http://www.britishmuseum.org) – https://www.gov.uk/government/organisations/british-museum --- app/config.py | 1 + tests/app/main/test_validators.py | 1 + 2 files changed, 2 insertions(+) diff --git a/app/config.py b/app/config.py index 04b0bbc59..3fe195776 100644 --- a/app/config.py +++ b/app/config.py @@ -97,6 +97,7 @@ class Config(object): r"gov\.wales", r"biglotteryfund\.org\.uk", r"marinemanagement\.org\.uk", + r"britishmuseum\.org", ] LOGO_UPLOAD_BUCKET_NAME = 'public-logos-local' diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index f16e71a2c..f7a6f959d 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -100,6 +100,7 @@ def _gen_mock_field(x): 'test@acas.org.uk', 'test@biglotteryfund.org.uk', 'test@marinemanagement.org.uk', + 'test@britishmuseum.org', ]) def test_valid_list_of_white_list_email_domains( client, From efe90f68307030d5788780fca92ae86a891c49e8 Mon Sep 17 00:00:00 2001 From: chrisw Date: Wed, 6 Dec 2017 12:19:50 +0000 Subject: [PATCH 07/23] Updated service manual link on using-notify page --- app/templates/views/using-notify.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/templates/views/using-notify.html b/app/templates/views/using-notify.html index d0d8d834c..b1e817f90 100644 --- a/app/templates/views/using-notify.html +++ b/app/templates/views/using-notify.html @@ -16,10 +16,10 @@

Using Notify

Writing and sending text messages, emails and letters

-

Check the Service Manual for guidance on how to:

+

Check the Service Manual for guidance on how to:

Trial mode

From b24c23fe3bdbfff370530438257037e5c5485b22 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 4 Jan 2018 10:22:37 +0000 Subject: [PATCH 08/23] Lint for print statements flake8-print is a flake8 plugin that checks for `print()` statements in Python files. This should save us having to manually spot these when reviewing pull requests. The `--enable=T` flag needs to be set until this bug is fixed: https://github.com/JBKahn/flake8-print/issues/27 --- app/commands.py | 2 +- requirements_for_test.txt | 1 + scripts/run_tests.sh | 2 +- tests/__init__.py | 2 -- 4 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/commands.py b/app/commands.py index c13dfee18..026be777a 100644 --- a/app/commands.py +++ b/app/commands.py @@ -4,7 +4,7 @@ from flask import current_app def list_routes(): """List URLs of all application routes.""" for rule in sorted(current_app.url_map.iter_rules(), key=lambda r: r.rule): - print("{:10} {}".format(", ".join(rule.methods - set(['OPTIONS', 'HEAD'])), rule.rule)) + print("{:10} {}".format(", ".join(rule.methods - set(['OPTIONS', 'HEAD'])), rule.rule)) # noqa def setup_commands(application): diff --git a/requirements_for_test.txt b/requirements_for_test.txt index b3196465e..a1da983f3 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -8,3 +8,4 @@ httpretty==0.8.14 beautifulsoup4==4.6.0 freezegun==0.3.9 flake8==3.5.0 +flake8-print==3.0.1 diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh index d8d6b5a39..2b5da0565 100755 --- a/scripts/run_tests.sh +++ b/scripts/run_tests.sh @@ -28,7 +28,7 @@ if [[ -z "$VIRTUAL_ENV" ]] && [[ -d venv ]]; then source ./venv/bin/activate fi -flake8 . +flake8 --enable=T . display_result $? 1 "Code style check" npm test diff --git a/tests/__init__.py b/tests/__init__.py index 262b008e5..b5ba0350d 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -368,7 +368,6 @@ def validate_route_permission(mocker, else: pytest.fail("Invalid method call {}".format(method)) if resp.status_code != response_code: - print(resp.status_code) pytest.fail("Invalid permissions set for endpoint {}".format(route)) return resp @@ -403,6 +402,5 @@ def validate_route_permission_with_client(mocker, else: pytest.fail("Invalid method call {}".format(method)) if resp.status_code != response_code: - print(resp.status_code) pytest.fail("Invalid permissions set for endpoint {}".format(route)) return resp From 22de67fa195f75739c59ba79e9fa3ca39d0134e9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 3 Jan 2018 13:30:51 +0000 Subject: [PATCH 09/23] Show all pages of a sent letter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because we weren’t specifying how many pages the letter has, it was defaulting to only showing the first page. This looked broken. --- app/main/views/notifications.py | 3 ++- tests/app/main/views/test_notifications.py | 25 +++++++++++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index ed6fe756b..d222059e4 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -14,7 +14,7 @@ from app import ( current_service ) from app.main import main -from app.template_previews import TemplatePreview +from app.template_previews import TemplatePreview, get_page_count_for_letter from app.utils import ( user_has_permissions, get_help_argument, @@ -40,6 +40,7 @@ def view_notification(service_id, notification_id): notification_id=notification_id, filetype='png', ), + page_count=get_page_count_for_letter(notification['template']), show_recipient=True, redact_missing_personalisation=True, ) diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index ff2e94890..2ef218748 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -109,13 +109,19 @@ def test_notification_page_doesnt_link_to_template_in_tour( @freeze_time("2016-01-01 01:01") -def test_notification_page_shows_status_of_letter_notification( +def test_notification_page_shows_page_for_letter_notification( client_request, mocker, fake_uuid, ): + count_of_pages = 3 + mock_get_notification(mocker, fake_uuid, template_type='letter') + mocker.patch( + 'app.main.views.notifications.get_page_count_for_letter', + return_value=count_of_pages + ) page = client_request.get( 'main.view_notification', @@ -131,6 +137,15 @@ def test_notification_page_shows_status_of_letter_notification( ) assert page.select('p.notification-status') == [] + letter_images = page.select('main img') + + assert len(letter_images) == count_of_pages + + for index in range(1, count_of_pages + 1): + assert page.select('img')[index]['src'].endswith( + '.png?page={}'.format(index) + ) + @pytest.mark.parametrize('filetype', [ 'pdf', 'png' @@ -193,6 +208,10 @@ def test_notification_page_has_link_to_send_another_for_sms( service_one['permissions'] = service_permissions mock_get_notification(mocker, fake_uuid, template_type=template_type) + mocker.patch( + 'app.main.views.notifications.get_page_count_for_letter', + return_value=1 + ) page = client_request.get( 'main.view_notification', @@ -237,6 +256,10 @@ def test_notification_page_has_link_to_download_letter( ): mock_get_notification(mocker, fake_uuid, template_type=template_type) + mocker.patch( + 'app.main.views.notifications.get_page_count_for_letter', + return_value=1 + ) page = client_request.get( 'main.view_notification', From 9e7092404292767b8d3121942570837151a16c77 Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Thu, 4 Jan 2018 16:56:54 +0000 Subject: [PATCH 10/23] 128-131 and 53 to 55 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ABCi - Aneurin Bevan UHB – Aneurin Bevan University Health Board * Research – Driver & Vehicle Standards Agency * Tunbridge Wells Borough Council  – Tunbridge Wells Borough Council --- app/templates/views/signedout.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index 051ea14f7..5c5968a0d 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -120,12 +120,12 @@
-
128
+
131
services
-
53
+
55
organisations
From 3e9e45ee296fc47bc83c4f1255721f738598a6ca Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Fri, 5 Jan 2018 12:46:24 +0000 Subject: [PATCH 11/23] Update pytest from 3.3.1 to 3.3.2 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index a1da983f3..7e0d0651e 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,5 +1,5 @@ -r requirements.txt -pytest==3.3.1 +pytest==3.3.2 pytest-mock==1.6.3 pytest-cov==2.5.1 pytest-xdist==1.21.0 From 0a7fe52e83d394502ec1845be766974033da9c5d Mon Sep 17 00:00:00 2001 From: chrisw Date: Fri, 5 Jan 2018 14:23:19 +0000 Subject: [PATCH 12/23] make content links in Notify consistent --- .../views/service-settings/request-to-go-live.html | 6 +++--- app/templates/views/terms-of-use.html | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/templates/views/service-settings/request-to-go-live.html b/app/templates/views/service-settings/request-to-go-live.html index 0a7f087ca..48cb4f3d6 100644 --- a/app/templates/views/service-settings/request-to-go-live.html +++ b/app/templates/views/service-settings/request-to-go-live.html @@ -24,10 +24,10 @@ settings page
  • added the templates you want to start with, making sure they follow our - design patterns, - style guide + design patterns, + style guide and - information security guidelines + information security guidelines
    • diff --git a/app/templates/views/terms-of-use.html b/app/templates/views/terms-of-use.html index 9add85681..8956b9cfb 100644 --- a/app/templates/views/terms-of-use.html +++ b/app/templates/views/terms-of-use.html @@ -39,7 +39,7 @@ Terms of use
  • get the right levels of consent (to send messages and to use data)
  • not send unsolicited messages, only ones related to a transaction or something the user has subscribed to be updated about (check the Service Manual if you’re not sure)
  • - send messages that meet the GOV.UK design patterns, style guide and information security guidelines
    • + send messages that meet the GOV.UK design patterns, style guide and information security guidelines
  • not send messages containing any personally or commercially sensitive information
  • check that the data you add to Notify is accurate and complies with Data Protection Act principles
    • From 17ea920be4ae119a5828ae827f4528c30b92082e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 8 Jan 2018 15:48:25 +0000 Subject: [PATCH 13/23] Update previous/next navigation to match GOV.UK The pagination pattern on GOV.UK changed in: https://github.com/alphagov/static/pull/1051 We should update ours to match because: - consistency - the new icons looks better --- .../vendor/previous-next-navigation.scss | 116 ++++++++---------- .../components/previous-next-navigation.html | 14 ++- 2 files changed, 62 insertions(+), 68 deletions(-) diff --git a/app/assets/stylesheets/components/vendor/previous-next-navigation.scss b/app/assets/stylesheets/components/vendor/previous-next-navigation.scss index a1cf5e0ea..5f8f9f4d9 100644 --- a/app/assets/stylesheets/components/vendor/previous-next-navigation.scss +++ b/app/assets/stylesheets/components/vendor/previous-next-navigation.scss @@ -1,6 +1,6 @@ /* Taken from the GOV.UK component at -https://github.com/alphagov/static/blob/a9d462e71709d2ff6348bcce7e8c625af2b86114/app/assets/stylesheets/govuk-component/_previous-and-next-navigation.scss +https://github.com/alphagov/static/blob/3d93a762b9d7af54615c77ae3e479131c03b8175/app/assets/stylesheets/govuk-component/_previous-and-next-navigation.scss and https://github.com/alphagov/static/blob/da8aeeaa749093eab30286d7fc9f965533b66f47/app/assets/stylesheets/styleguide/_conditionals2.scss */ @@ -31,12 +31,11 @@ $is-ie: false !default; } .govuk-previous-and-next-navigation { - - @include media-down(mobile) { - margin: 2em 0 0 0; - } - display: block; + margin-top: $gutter; + margin-bottom: $gutter; + margin-left: -$gutter-half; + margin-right: -$gutter-half; ul { margin: 0; @@ -50,18 +49,17 @@ $is-ie: false !default; text-align: right; margin: 0; padding: 0; - width: 49%; + width: 50%; a { - - @include ie-lte(7) { - height: 4.5em; - } - display: block; - color: $link-colour; + padding: $gutter-half; text-decoration: none; + &:visited { + color: $link-colour; + } + &:hover, &:active { background-color: $canvas-colour; @@ -69,61 +67,47 @@ $is-ie: false !default; .pagination-part-title { @include core-27($line-height: (33.75 / 27)); - margin-bottom: 0.1em; display: block; } - - } - - &.next-page { - float: right; - text-align: right; - } - - &.next-page a:before { - background: transparent file-url("arrow-sprite.png") no-repeat -102px -11px; - margin: -4px -32px 0 0; - display: block; - float: right; - width: 30px; - height: 38px; - content: " "; - } - - &.previous-page a:before { - background: transparent file-url("arrow-sprite.png") no-repeat -20px -11px; - margin: -4px 0 0 -32px; - display: block; - float: left; - width: 30px; - height: 38px; - content: " "; - } - - &.previous-page { - float: left; - text-align: left; - } - - &.previous-page a { - padding: 0.75em 0 0.75em 3em; - } - - &.next-page a { - padding: 0.75em 3em 0.75em 0; - } - - @include media-down(mobile) { - &.previous-page, - &.next-page { - float: none; - width: 100%; - } - - &.next-page a { - text-align: right; - } } } -} \ No newline at end of file + .previous-page { + float: left; + text-align: left; + } + + .next-page { + float: right; + text-align: right; + } + + @include media-down(mobile) { + .previous-page, + .next-page { + float: none; + width: 100%; + } + + .next-page a { + text-align: right; + } + } + + .pagination-icon { + display: inline-block; + margin-bottom: 1px; + height: .482em; + width: .63em; + } + + .pagination-label { + display: inline-block; + margin-top: 0.1em; + text-decoration: underline; + + &:empty { + display: none; + } + } +} diff --git a/app/templates/components/previous-next-navigation.html b/app/templates/components/previous-next-navigation.html index 525555396..e7102d4a0 100644 --- a/app/templates/components/previous-next-navigation.html +++ b/app/templates/components/previous-next-navigation.html @@ -4,7 +4,12 @@ {% if previous_page %}
  • - {{previous_page['title']}} + + + + + {{previous_page['title']}} + {{previous_page['label']}}
    • @@ -12,7 +17,12 @@ {% if next_page %}
    • From ab55bf49b2977d7c0b94ac5bec7b6b5a5f64467a Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Tue, 9 Jan 2018 14:14:47 +0000 Subject: [PATCH 14/23] Add generate_manifest.py script and update deployment tasks Brings in the new environment variables deployment process introduced in alphagov/notifications-api#1543. The script is a copy of the API one and make steps are modified to fit with the existing admin deployment targets. --- Makefile | 18 +++++++++-- scripts/generate_manifest.py | 63 ++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 3 deletions(-) create mode 100755 scripts/generate_manifest.py diff --git a/Makefile b/Makefile index b45f3fbd0..1aafc9d82 100644 --- a/Makefile +++ b/Makefile @@ -25,6 +25,9 @@ CF_SPACE ?= ${DEPLOY_ENV} CF_HOME ?= ${HOME} $(eval export CF_HOME) +CF_MANIFEST_FILE ?= manifest-${CF_SPACE}.yml +NOTIFY_CREDENTIALS ?= ~/.notify-credentials + .PHONY: help help: @cat $(MAKEFILE_LIST) | grep -E '^[a-zA-Z_-]+:.*?## .*$$' | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}' @@ -167,12 +170,21 @@ cf-login: ## Log in to Cloud Foundry @echo "Logging in to Cloud Foundry on ${CF_API}" @cf login -a "${CF_API}" -u ${CF_USERNAME} -p "${CF_PASSWORD}" -o "${CF_ORG}" -s "${CF_SPACE}" +.PHONY: generate-manifest +generate-manifest: + $(if ${CF_SPACE},,$(error Must specify CF_SPACE)) + $(if $(shell which gpg2), $(eval export GPG=gpg2), $(eval export GPG=gpg)) + $(if ${GPG_PASSPHRASE_TXT}, $(eval export DECRYPT_CMD=echo -n $$$${GPG_PASSPHRASE_TXT} | ${GPG} --quiet --batch --passphrase-fd 0 --pinentry-mode loopback -d), $(eval export DECRYPT_CMD=${GPG} --quiet --batch -d)) + + @./scripts/generate_manifest.py ${CF_MANIFEST_FILE} \ + <(${DECRYPT_CMD} ${NOTIFY_CREDENTIALS}/credentials/${CF_SPACE}/paas/environment-variables.gpg) + .PHONY: cf-deploy cf-deploy: ## Deploys the app to Cloud Foundry $(if ${CF_SPACE},,$(error Must specify CF_SPACE)) @cf app --guid notify-admin || exit 1 cf rename notify-admin notify-admin-rollback - cf push -f manifest-${CF_SPACE}.yml + cf push -f <(make -s generate-manifest) cf scale -i $$(cf curl /v2/apps/$$(cf app --guid notify-admin-rollback) | jq -r ".entity.instances" 2>/dev/null || echo "1") notify-admin cf stop notify-admin-rollback cf delete -f notify-admin-rollback @@ -180,7 +192,7 @@ cf-deploy: ## Deploys the app to Cloud Foundry .PHONY: cf-deploy-prototype cf-deploy-prototype: cf-target ## Deploys the app to Cloud Foundry $(if ${CF_SPACE},,$(error Must specify CF_SPACE)) - cf push -f manifest-prototype-${CF_SPACE}.yml + cf push -f <(make -s CF_MANIFEST_FILE=manifest-prototype-${CF_SPACE}.yml generate-manifest) .PHONY: cf-rollback cf-rollback: ## Rollbacks the app to the previous release @@ -191,7 +203,7 @@ cf-rollback: ## Rollbacks the app to the previous release .PHONY: cf-push cf-push: - cf push -f manifest-${CF_SPACE}.yml + cf push -f <(make -s generate-manifest) .PHONY: cf-target cf-target: check-env-vars diff --git a/scripts/generate_manifest.py b/scripts/generate_manifest.py new file mode 100755 index 000000000..ce7982763 --- /dev/null +++ b/scripts/generate_manifest.py @@ -0,0 +1,63 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- + +import os +import sys + +import json +import yaml + + +def merge_dicts(a, b): + if not (isinstance(a, dict) and isinstance(b, dict)): + raise ValueError("Error merging variables: '{}' and '{}'".format( + type(a).__name__, type(b).__name__ + )) + + result = a.copy() + for key, val in b.items(): + if isinstance(result.get(key), dict): + result[key] = merge_dicts(a[key], b[key]) + else: + result[key] = val + + return result + + +def load_manifest(manifest_file): + with open(manifest_file) as f: + manifest = yaml.load(f) + + if 'inherit' in manifest: + inherit_file = os.path.join(os.path.dirname(manifest_file), manifest.pop('inherit')) + manifest = merge_dicts(load_manifest(inherit_file), manifest) + + return manifest + + +def load_variables(vars_files): + variables = {} + for vars_file in vars_files: + with open(vars_file) as f: + variables = merge_dicts(variables, yaml.load(f)) + + return { + k.upper(): json.dumps(v) if isinstance(v, (dict, list)) else v + for k, v in variables.items() + } + + +def paas_manifest(manifest_file, *vars_files): + manifest = load_manifest(manifest_file) + variables = load_variables(vars_files) + + for key in manifest.get('env', {}): + if key in variables: + manifest['env'][key] = variables[key] + + return yaml.dump(manifest, default_flow_style=False, allow_unicode=True) + + +if __name__ == "__main__": + print('---') # noqa + print(paas_manifest(*sys.argv[1:])) # noqa From c550c686d11c3586eeb7a935cf918f4ebaf6bb07 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Tue, 9 Jan 2018 14:17:03 +0000 Subject: [PATCH 15/23] Replace manifest user-provided services with environment variables Variable values are populated from the credentials by the `generate-manifest` make target. --- manifest-base.yml | 26 ++++++++++++++++++++------ manifest-preview.yml | 5 ----- manifest-prototype-base.yml | 26 ++++++++++++++++++++------ manifest-staging.yml | 5 ----- 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/manifest-base.yml b/manifest-base.yml index 9f6d0d60f..26a167743 100644 --- a/manifest-base.yml +++ b/manifest-base.yml @@ -2,16 +2,30 @@ buildpack: python_buildpack command: scripts/run_app_paas.sh gunicorn -c /home/vcap/app/gunicorn_config.py --error-logfile /home/vcap/logs/gunicorn_error.log -w 5 -b 0.0.0.0:$PORT application -services: - - notify-aws - - notify-config - - notify-template-preview - - hosted-graphite - - deskpro instances: 1 memory: 1G env: NOTIFY_APP_NAME: admin + # Credentials variables + ADMIN_CLIENT_SECRET: null + ADMIN_BASE_URL: null + API_HOST_NAME: null + DANGEROUS_SALT: null + SECRET_KEY: null + ROUTE_SECRET_KEY_1: null + ROUTE_SECRET_KEY_2: null + + AWS_ACCESS_KEY_ID: null + AWS_SECRET_ACCESS_KEY: null + + STATSD_PREFIX: null + + DESKPRO_API_HOST: null + DESKPRO_API_KEY: null + + TEMPLATE_PREVIEW_API_HOST: null + TEMPLATE_PREVIEW_API_KEY: null + applications: - name: notify-admin diff --git a/manifest-preview.yml b/manifest-preview.yml index 34f46f22c..8573b785b 100644 --- a/manifest-preview.yml +++ b/manifest-preview.yml @@ -3,11 +3,6 @@ inherit: manifest-base.yml services: - - notify-aws - - notify-config - - notify-template-preview - - hosted-graphite - - deskpro - logit-ssl-syslog-drain routes: diff --git a/manifest-prototype-base.yml b/manifest-prototype-base.yml index cf28cbb6a..6003594b7 100644 --- a/manifest-prototype-base.yml +++ b/manifest-prototype-base.yml @@ -2,16 +2,30 @@ buildpack: python_buildpack command: scripts/run_app_paas.sh gunicorn -w 5 -b 0.0.0.0:$PORT application -services: - - notify-aws - - notify-config - - notify-template-preview - - hosted-graphite - - deskpro instances: 1 memory: 1G env: NOTIFY_APP_NAME: admin + # Credentials variables + ADMIN_CLIENT_SECRET: null + ADMIN_BASE_URL: null + API_HOST_NAME: null + DANGEROUS_SALT: null + SECRET_KEY: null + ROUTE_SECRET_KEY_1: null + ROUTE_SECRET_KEY_2: null + + AWS_ACCESS_KEY_ID: null + AWS_SECRET_ACCESS_KEY: null + + STATSD_PREFIX: null + + DESKPRO_API_HOST: null + DESKPRO_API_KEY: null + + TEMPLATE_PREVIEW_API_HOST: null + TEMPLATE_PREVIEW_API_KEY: null + applications: - name: notify-admin-prototype diff --git a/manifest-staging.yml b/manifest-staging.yml index c0f1d862f..ed4119be7 100644 --- a/manifest-staging.yml +++ b/manifest-staging.yml @@ -7,11 +7,6 @@ routes: - route: www.staging-notify.works services: - - notify-aws - - notify-config - - notify-template-preview - - hosted-graphite - - deskpro - logit-ssl-syslog-drain instances: 2 From 8346e1b8be74ddeea481da06ca0b0ba715f73fd3 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Tue, 9 Jan 2018 14:18:32 +0000 Subject: [PATCH 16/23] Remove VCAP_SERVICES parsing code We're no longer using user-provided services for application secrets so we can remove the cloudfoundry_config code responsible for parsing VCAP_SERVICES. --- app/cloudfoundry_config.py | 47 --------- tests/app/test_cloudfoundry_config.py | 141 +------------------------- 2 files changed, 2 insertions(+), 186 deletions(-) diff --git a/app/cloudfoundry_config.py b/app/cloudfoundry_config.py index 6d55acf37..97004b6ad 100644 --- a/app/cloudfoundry_config.py +++ b/app/cloudfoundry_config.py @@ -8,53 +8,6 @@ import json def extract_cloudfoundry_config(): - vcap_services = json.loads(os.environ['VCAP_SERVICES']) - - set_config_env_vars(vcap_services) - - -def set_config_env_vars(vcap_services): vcap_application = json.loads(os.environ.get('VCAP_APPLICATION')) os.environ['NOTIFY_ENVIRONMENT'] = vcap_application['space_name'] os.environ['NOTIFY_LOG_PATH'] = '/home/vcap/logs/app.log' - - for s in vcap_services['user-provided']: - if s['name'] == 'notify-config': - extract_notify_config(s) - elif s['name'] == 'notify-aws': - extract_notify_aws_config(s) - elif s['name'] == 'hosted-graphite': - extract_hosted_graphite_config(s) - elif s['name'] == 'deskpro': - extract_deskpro_config(s) - elif s['name'] == 'notify-template-preview': - extract_template_preview_config(s) - - -def extract_notify_config(notify_config): - os.environ['ADMIN_CLIENT_SECRET'] = notify_config['credentials']['admin_client_secret'] - os.environ['API_HOST_NAME'] = notify_config['credentials']['api_host_name'] - os.environ['ADMIN_BASE_URL'] = notify_config['credentials']['admin_base_url'] - os.environ['SECRET_KEY'] = notify_config['credentials']['secret_key'] - os.environ['DANGEROUS_SALT'] = notify_config['credentials']['dangerous_salt'] - os.environ['ROUTE_SECRET_KEY_1'] = notify_config['credentials']['route_secret_key_1'] - os.environ['ROUTE_SECRET_KEY_2'] = notify_config['credentials']['route_secret_key_2'] - - -def extract_notify_aws_config(aws_config): - os.environ['AWS_ACCESS_KEY_ID'] = aws_config['credentials']['aws_access_key_id'] - os.environ['AWS_SECRET_ACCESS_KEY'] = aws_config['credentials']['aws_secret_access_key'] - - -def extract_hosted_graphite_config(hosted_graphite_config): - os.environ['STATSD_PREFIX'] = hosted_graphite_config['credentials']['statsd_prefix'] - - -def extract_deskpro_config(deskpro_config): - os.environ['DESKPRO_API_HOST'] = deskpro_config['credentials']['api_host'] - os.environ['DESKPRO_API_KEY'] = deskpro_config['credentials']['api_key'] - - -def extract_template_preview_config(template_preview_config): - os.environ['TEMPLATE_PREVIEW_API_HOST'] = template_preview_config['credentials']['api_host'] - os.environ['TEMPLATE_PREVIEW_API_KEY'] = template_preview_config['credentials']['api_key'] diff --git a/tests/app/test_cloudfoundry_config.py b/tests/app/test_cloudfoundry_config.py index 22b8f93c0..dfd2ccdf7 100644 --- a/tests/app/test_cloudfoundry_config.py +++ b/tests/app/test_cloudfoundry_config.py @@ -1,92 +1,12 @@ import os -import json import pytest -from app.cloudfoundry_config import extract_cloudfoundry_config, set_config_env_vars +from app.cloudfoundry_config import extract_cloudfoundry_config @pytest.fixture -def notify_config(): - return { - 'name': 'notify-config', - 'credentials': { - 'api_host_name': 'api host name', - 'admin_base_url': 'admin base url', - 'admin_client_secret': 'admin client secret', - 'secret_key': 'secret key', - 'dangerous_salt': 'dangerous salt', - 'route_secret_key_1': 'key 1', - 'route_secret_key_2': 'key 2', - } - } - - -@pytest.fixture -def aws_config(): - return { - 'name': 'notify-aws', - 'credentials': { - 'aws_access_key_id': 'aws access key id', - 'aws_secret_access_key': 'aws secret access key', - } - } - - -@pytest.fixture -def hosted_graphite_config(): - return { - 'name': 'hosted-graphite', - 'credentials': { - 'statsd_prefix': 'statsd prefix' - } - } - - -@pytest.fixture -def deskpro_config(): - return { - 'name': 'deskpro', - 'credentials': { - 'api_host': 'deskpro api host', - 'api_key': 'deskpro api key' - } - } - - -@pytest.fixture -def template_preview_config(): - return { - 'name': 'notify-template-preview', - 'credentials': { - 'api_host': 'template-preview api host', - 'api_key': 'template-preview api key' - } - } - - -@pytest.fixture -def cloudfoundry_config( - notify_config, - aws_config, - hosted_graphite_config, - deskpro_config, - template_preview_config, -): - return { - 'user-provided': [ - notify_config, - aws_config, - hosted_graphite_config, - deskpro_config, - template_preview_config, - ] - } - - -@pytest.fixture -def cloudfoundry_environ(monkeypatch, cloudfoundry_config): - monkeypatch.setenv('VCAP_SERVICES', json.dumps(cloudfoundry_config)) +def cloudfoundry_environ(monkeypatch): monkeypatch.setenv('VCAP_APPLICATION', '{"space_name":"🚀🌌"}') @@ -96,60 +16,3 @@ def test_extract_cloudfoundry_config_populates_other_vars(): assert os.environ['NOTIFY_ENVIRONMENT'] == '🚀🌌' assert os.environ['NOTIFY_LOG_PATH'] == '/home/vcap/logs/app.log' - - -@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') -def test_set_config_env_vars_ignores_unknown_configs(cloudfoundry_config): - cloudfoundry_config['foo'] = {'credentials': {'foo': 'foo'}} - cloudfoundry_config['user-provided'].append({ - 'name': 'bar', 'credentials': {'bar': 'bar'} - }) - - set_config_env_vars(cloudfoundry_config) - - assert 'foo' not in os.environ - assert 'bar' not in os.environ - - -@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') -def test_notify_config(): - extract_cloudfoundry_config() - - assert os.environ['API_HOST_NAME'] == 'api host name' - assert os.environ['ADMIN_BASE_URL'] == 'admin base url' - assert os.environ['ADMIN_CLIENT_SECRET'] == 'admin client secret' - assert os.environ['SECRET_KEY'] == 'secret key' - assert os.environ['DANGEROUS_SALT'] == 'dangerous salt' - assert os.environ['ROUTE_SECRET_KEY_1'] == 'key 1' - assert os.environ['ROUTE_SECRET_KEY_2'] == 'key 2' - - -@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') -def test_aws_config(): - extract_cloudfoundry_config() - - assert os.environ['AWS_ACCESS_KEY_ID'] == 'aws access key id' - assert os.environ['AWS_SECRET_ACCESS_KEY'] == 'aws secret access key' - - -@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') -def test_hosted_graphite_config(): - extract_cloudfoundry_config() - - assert os.environ['STATSD_PREFIX'] == 'statsd prefix' - - -@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') -def test_deskpro_config(): - extract_cloudfoundry_config() - - assert os.environ['DESKPRO_API_HOST'] == 'deskpro api host' - assert os.environ['DESKPRO_API_KEY'] == 'deskpro api key' - - -@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') -def test_template_preview_config(): - extract_cloudfoundry_config() - - assert os.environ['TEMPLATE_PREVIEW_API_HOST'] == 'template-preview api host' - assert os.environ['TEMPLATE_PREVIEW_API_KEY'] == 'template-preview api key' From 945c967250b61e192930421fa41109057a4b3a13 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Tue, 9 Jan 2018 14:20:18 +0000 Subject: [PATCH 17/23] Don't parse AWS credentials from VCAP_SERVICES in run_app_paas AWS credentials are provided in the environment variables directly, so we don't need to parse them from VCAP_SERVICES --- scripts/run_app_paas.sh | 3 --- 1 file changed, 3 deletions(-) diff --git a/scripts/run_app_paas.sh b/scripts/run_app_paas.sh index 499cfeb2b..9abb9fcae 100755 --- a/scripts/run_app_paas.sh +++ b/scripts/run_app_paas.sh @@ -18,9 +18,6 @@ function check_params { function configure_aws_logs { aws configure set plugins.cwlogs cwlogs - export AWS_ACCESS_KEY_ID=$(echo ${VCAP_SERVICES} | jq -r '.["user-provided"][]|select(.name=="notify-aws")|.credentials.aws_access_key_id') - export AWS_SECRET_ACCESS_KEY=$(echo ${VCAP_SERVICES} | jq -r '.["user-provided"][]|select(.name=="notify-aws")|.credentials.aws_secret_access_key') - cat > /home/vcap/app/awslogs.conf << EOF [general] state_file = /home/vcap/logs/awslogs-state From 0088d588e1a64841a62afc778a6cdbab61e0c8ae Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Tue, 9 Jan 2018 14:41:36 +0000 Subject: [PATCH 18/23] Use VCAP_APPLICATION to detect CloudFoundry environment VCAP_SERVICES is not set on PaaS if no services are bound to the application, so we need to check for VCAP_APPLICATION to parse the application name and environment. --- app/config.py | 4 ++-- tests/app/test_config.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/config.py b/app/config.py index 3fe195776..c4443ee1f 100644 --- a/app/config.py +++ b/app/config.py @@ -1,8 +1,8 @@ import os -if os.environ.get('VCAP_SERVICES'): - # on cloudfoundry, config is a json blob in VCAP_SERVICES - unpack it, and populate +if os.environ.get('VCAP_APPLICATION'): + # on cloudfoundry, config is a json blob in VCAP_APPLICATION - unpack it, and populate # standard environment variables from it from app.cloudfoundry_config import extract_cloudfoundry_config extract_cloudfoundry_config() diff --git a/tests/app/test_config.py b/tests/app/test_config.py index 87cc10aed..9ea0f3c72 100644 --- a/tests/app/test_config.py +++ b/tests/app/test_config.py @@ -26,7 +26,7 @@ def reload_config(): def test_load_cloudfoundry_config_if_available(monkeypatch, reload_config): os.environ['API_HOST_NAME'] = 'env' - monkeypatch.setenv('VCAP_SERVICES', 'some json blob') + monkeypatch.setenv('VCAP_APPLICATION', 'some json blob') with mock.patch('app.cloudfoundry_config.extract_cloudfoundry_config', side_effect=cf_conf) as cf_config: # reload config so that its module level code (ie: all of it) is re-instantiated @@ -41,7 +41,7 @@ def test_load_cloudfoundry_config_if_available(monkeypatch, reload_config): def test_load_config_if_cloudfoundry_not_available(monkeypatch, reload_config): os.environ['API_HOST_NAME'] = 'env' - monkeypatch.delenv('VCAP_SERVICES', raising=False) + monkeypatch.delenv('VCAP_APPLICATION', raising=False) with mock.patch('app.cloudfoundry_config.extract_cloudfoundry_config') as cf_config: # reload config so that its module level code (ie: all of it) is re-instantiated From b9f7fa1f6f97307222483c1266d9ef7da03b0ab2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 5 Jan 2018 16:27:51 +0000 Subject: [PATCH 19/23] Replace cookie implementation with flask builtin secure cookie. However, by default, session cookies don't expire (only cleared out by the end user's browser). This is dumb. You'd think, given that there's `config['PERMANENT_SESSION_LIFETIME']`, that you'd enable permanent sessions in the config too - but no, you have to declare it for each request. session.permanent is also, helpfully, a way of saying that the session isn't permanent - in that, it will expire on its own, as opposed to being controlled by the browser's session. Because session is a proxy, it's only accessible from within a request context, so we need to set this before every request :roll_eyes: http://flask.pocoo.org/docs/0.12/api/#flask.session https://stackoverflow.com/questions/34118093/flask-permanent-session-where-to-define-them --- app/__init__.py | 13 +++++++-- app/its_dangerous_session.py | 53 ------------------------------------ 2 files changed, 10 insertions(+), 56 deletions(-) delete mode 100644 app/its_dangerous_session.py diff --git a/app/__init__.py b/app/__init__.py index 7669fbc73..48a664431 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -38,7 +38,6 @@ from werkzeug.local import LocalProxy from app import proxy_fix from app.config import configs from app.asset_fingerprinter import AssetFingerprinter -from app.its_dangerous_session import ItsdangerousSessionInterface from app.notify_client.service_api_client import ServiceAPIClient from app.notify_client.api_key_api_client import ApiKeyApiClient from app.notify_client.invite_api_client import InviteApiClient @@ -124,8 +123,6 @@ def create_app(application): proxy_fix.init_app(application) - application.session_interface = ItsdangerousSessionInterface() - add_template_filters(application) register_errorhandlers(application) @@ -139,6 +136,16 @@ def init_app(application): application.before_request(load_service_before_request) application.before_request(request_helper.check_proxy_header_before_request) + @application.before_request + def make_session_permanent(): + # this is dumb. You'd think, given that there's `config['PERMANENT_SESSION_LIFETIME']`, that you'd enable + # permanent sessions in the config too - but no, you have to declare it for each request. + # https://stackoverflow.com/questions/34118093/flask-permanent-session-where-to-define-them + # session.permanent is also, helpfully, a way of saying that the session isn't permanent - in that, it will + # expire on its own, as opposed to being controlled by the browser's session. Because session is a proxy, it's + # only accessible from within a request context, so we need to set this before every request :rolls_eyes: + session.permanent = True + @application.context_processor def _attach_current_service(): return {'current_service': current_service} diff --git a/app/its_dangerous_session.py b/app/its_dangerous_session.py deleted file mode 100644 index e16cecc55..000000000 --- a/app/its_dangerous_session.py +++ /dev/null @@ -1,53 +0,0 @@ -from datetime import timedelta, datetime - -from werkzeug.datastructures import CallbackDict -from flask.sessions import SessionInterface, SessionMixin -from itsdangerous import URLSafeTimedSerializer, BadSignature - - -class ItsdangerousSession(CallbackDict, SessionMixin): - def __init__(self, initial=None): - def on_update(self): - self.modified = True - - CallbackDict.__init__(self, initial, on_update) - self.modified = False - - -class ItsdangerousSessionInterface(SessionInterface): - session_class = ItsdangerousSession - - def get_serializer(self, app): - salt = app.config.get('DANGEROUS_SALT') - if not app.secret_key: - return None - return URLSafeTimedSerializer(app.secret_key, - salt=salt) - - def open_session(self, app, request): - s = self.get_serializer(app) - if s is None: - return None - val = request.cookies.get(app.session_cookie_name) - if not val: - return self.session_class() - max_age = app.permanent_session_lifetime.total_seconds() - try: - data = s.loads(val, max_age=max_age) - return self.session_class(data) - except BadSignature: - return self.session_class() - - def save_session(self, app, session, response): - domain = self.get_cookie_domain(app) - if not session: - if session.modified: - response.delete_cookie(app.session_cookie_name, - domain=domain) - return - session.permanent = True - expires = datetime.utcnow() + timedelta(seconds=app.config.get('PERMANENT_SESSION_LIFETIME')) - val = self.get_serializer(app).dumps(dict(session)) - response.set_cookie(app.session_cookie_name, val, - expires=expires, httponly=True, - domain=domain, secure=app.config.get('SESSION_COOKIE_SECURE')) From c780c94fb60e975914687f297f98269ab08e007c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 10 Jan 2018 10:47:40 +0000 Subject: [PATCH 20/23] derry strabane council email whitelist --- app/config.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/config.py b/app/config.py index c4443ee1f..547821b47 100644 --- a/app/config.py +++ b/app/config.py @@ -98,6 +98,7 @@ class Config(object): r"biglotteryfund\.org\.uk", r"marinemanagement\.org\.uk", r"britishmuseum\.org", + r"derrystrabane\.com", ] LOGO_UPLOAD_BUCKET_NAME = 'public-logos-local' From 224b9e509870587a10552639c89caf9d97508733 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 10 Jan 2018 10:48:58 +0000 Subject: [PATCH 21/23] Update test_validators.py --- tests/app/main/test_validators.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index f7a6f959d..d1fd670fa 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -136,6 +136,7 @@ def test_valid_list_of_white_list_email_domains( 'test@police.test.uk', 'test@ucds.com', 'test@123bl.uk', + 'test@derrystrabane.com', ]) def test_invalid_list_of_white_list_email_domains( client, From b27eacb03b7910e4c7c197bb8113f27093bd9693 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 10 Jan 2018 10:56:21 +0000 Subject: [PATCH 22/23] oops --- tests/app/main/test_validators.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index d1fd670fa..43f094cbc 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -101,6 +101,7 @@ def _gen_mock_field(x): 'test@biglotteryfund.org.uk', 'test@marinemanagement.org.uk', 'test@britishmuseum.org', + 'test@derrystrabane.com', ]) def test_valid_list_of_white_list_email_domains( client, @@ -136,7 +137,6 @@ def test_valid_list_of_white_list_email_domains( 'test@police.test.uk', 'test@ucds.com', 'test@123bl.uk', - 'test@derrystrabane.com', ]) def test_invalid_list_of_white_list_email_domains( client, From 7271d4fbde857300f3e1b3860adffe59226a341d Mon Sep 17 00:00:00 2001 From: chrisw Date: Wed, 3 Jan 2018 10:44:36 +0000 Subject: [PATCH 23/23] Allow letter templates to select the default contact block from the list --- app/main/forms.py | 10 + app/main/views/notifications.py | 3 + app/main/views/service_settings.py | 4 + app/main/views/templates.py | 68 +- app/notify_client/service_api_client.py | 10 + app/template_previews.py | 2 +- app/templates/views/templates/_template.html | 6 +- .../views/templates/set-template-sender.html | 32 + app/utils.py | 3 +- tests/__init__.py | 8 + tests/app/main/views/test_api_integration.py | 639 ++++++++++++++++++ tests/app/main/views/test_service_settings.py | 28 + tests/app/main/views/test_templates.py | 83 ++- tests/app/test_template_previews.py | 8 +- tests/conftest.py | 11 + 15 files changed, 875 insertions(+), 40 deletions(-) create mode 100644 app/templates/views/templates/set-template-sender.html create mode 100644 tests/app/main/views/test_api_integration.py diff --git a/app/main/forms.py b/app/main/forms.py index ea89af61d..9901cb89a 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -839,3 +839,13 @@ class SetSenderForm(StripWhitespaceForm): self.sender.label.text = kwargs['sender_label'] sender = RadioField() + + +class SetTemplateSenderForm(StripWhitespaceForm): + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.sender.choices = kwargs['sender_choices'] + self.sender.label.text = 'Select your sender' + + sender = RadioField() diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index d222059e4..aa7e82ccc 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -31,6 +31,8 @@ from app.utils import ( @user_has_permissions('view_activity', admin_override=True) def view_notification(service_id, notification_id): notification = notification_api_client.get_notification(service_id, str(notification_id)) + notification['template'].update({'reply_to_text': notification['reply_to_text']}) + template = get_template( notification['template'], current_service, @@ -82,6 +84,7 @@ def view_letter_notification_as_preview(service_id, notification_id, filetype): abort(404) notification = notification_api_client.get_notification(service_id, notification_id) + notification['template'].update({'reply_to_text': notification['reply_to_text']}) template = get_template( notification['template'], diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 7631b0f39..e1e11373b 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -553,6 +553,10 @@ def service_add_letter_contact(service_id): contact_block=form.letter_contact_block.data.replace('\r', '') or None, is_default=first_contact_block if first_contact_block else form.is_default.data ) + if request.args.get('from_template'): + return redirect( + url_for('.set_template_sender', service_id=service_id, template_id=request.args.get('from_template')) + ) return redirect(url_for('.service_letter_contact_details', service_id=service_id)) return render_template( 'views/service-settings/letter-contact/add.html', diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 840483f0f..90f7d6b82 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -12,7 +12,9 @@ from flask import ( ) from flask_login import login_required, current_user from dateutil.parser import parse +from markupsafe import Markup +from notifications_utils.formatters import nl2br from notifications_utils.recipients import first_column_headings from notifications_utils.template import LetterDVLATemplate from notifications_python_client.errors import HTTPError @@ -26,8 +28,9 @@ from app.main.forms import ( EmailTemplateForm, LetterTemplateForm, SearchTemplatesForm, + SetTemplateSenderForm, ) -from app.main.views.send import get_example_csv_rows +from app.main.views.send import get_example_csv_rows, get_sender_details from app import service_api_client, current_service, template_statistics_client @@ -181,6 +184,7 @@ def view_letter_template_preview(service_id, template_id, filetype): abort(404) db_template = service_api_client.get_service_template(service_id, template_id)['data'] + return TemplatePreview.from_database_object(db_template, filetype, page=request.args.get('page')) @@ -373,7 +377,8 @@ def edit_service_template(service_id, template_id): 'subject': subject, 'template_type': template['template_type'], 'id': template['id'], - 'process_type': form.process_type.data + 'process_type': form.process_type.data, + 'reply_to_text': template['reply_to_text'] }, current_service) template_change = get_template(template, current_service).compare_to(new_template) if template_change.placeholders_added and not request.form.get('confirm'): @@ -564,6 +569,65 @@ def view_template_versions(service_id, template_id): ) +@main.route('/services//templates//set-template-sender', methods=['GET', 'POST']) +@login_required +@user_has_permissions('manage_templates', admin_override=True) +def set_template_sender(service_id, template_id): + template = service_api_client.get_service_template(service_id, template_id)['data'] + sender_details = get_template_sender_form_dict(service_id, template) + no_senders = sender_details.get('no_senders', False) + + form = SetTemplateSenderForm( + sender=sender_details['current_choice'], + sender_choices=sender_details['value_and_label'], + ) + option_hints = {sender_details['default_sender']: '(Default)'} + + if form.validate_on_submit(): + service_api_client.update_service_template_sender( + service_id, + template_id, + form.sender.data if form.sender.data else None, + ) + return redirect(url_for('.view_template', service_id=service_id, template_id=template_id)) + + return render_template( + 'views/templates/set-template-sender.html', + form=form, + template_id=template_id, + no_senders=no_senders, + option_hints=option_hints + ) + + +def get_template_sender_form_dict(service_id, template): + context = { + 'email': { + 'field_name': 'email_address' + }, + 'letter': { + 'field_name': 'contact_block' + }, + 'sms': { + 'field_name': 'sms_sender' + } + }[template['template_type']] + + sender_format = context['field_name'] + service_senders = get_sender_details(service_id, template['template_type']) + context['default_sender'] = next( + (x['id'] for x in service_senders if x['is_default']), "Not set" + ) + if not service_senders: + context['no_senders'] = True + + context['value_and_label'] = [(sender['id'], Markup(nl2br(sender[sender_format]))) for sender in service_senders] + context['value_and_label'].insert(0, ('', 'Blank')) # Add blank option to start of list + + context['current_choice'] = template['service_letter_contact'] if template['service_letter_contact'] else '' + return context + + def get_last_use_message(template_name, template_statistics): try: most_recent_use = max( diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 2b7b6ead4..848da40b4 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -187,6 +187,16 @@ class ServiceAPIClient(NotifyAdminAPIClient): ), ) + def update_service_template_sender(self, service_id, template_id, reply_to): + data = { + 'reply_to': reply_to, + } + data = _attach_current_user(data) + return self.post( + "/service/{0}/template/{1}".format(service_id, template_id), + data + ) + def get_service_template(self, service_id, template_id, version=None, *params): """ Retrieve a service template. diff --git a/app/template_previews.py b/app/template_previews.py index a3f5970e8..e4493f3a4 100644 --- a/app/template_previews.py +++ b/app/template_previews.py @@ -8,7 +8,7 @@ class TemplatePreview: @classmethod def from_database_object(cls, template, filetype, values=None, page=None): data = { - 'letter_contact_block': current_service['letter_contact_block'], + 'letter_contact_block': template.get('reply_to_text', ''), 'template': template, 'values': values, 'dvla_org_id': current_service['dvla_organisation'], diff --git a/app/templates/views/templates/_template.html b/app/templates/views/templates/_template.html index 1dd6a41ab..70bce8a26 100644 --- a/app/templates/views/templates/_template.html +++ b/app/templates/views/templates/_template.html @@ -35,11 +35,7 @@
    {% if current_user.has_permissions(permissions=['manage_templates'], admin_override=True) and template.template_type == 'letter' %} Edit - {% if default_letter_contact_block_id %} - Edit - {% else %} - Edit - {% endif %} + Edit {% endif %} {{ template|string }}
    diff --git a/app/templates/views/templates/set-template-sender.html b/app/templates/views/templates/set-template-sender.html new file mode 100644 index 000000000..c25ea1789 --- /dev/null +++ b/app/templates/views/templates/set-template-sender.html @@ -0,0 +1,32 @@ +{% extends "withnav_template.html" %} +{% from "components/radios.html" import radios, branding_radios %} +{% from "components/page-footer.html" import page_footer %} + +{% block service_page_title %} + Set letter contact block +{% endblock %} + +{% block maincolumn_content %} + +

    Set letter contact block

    +
    +
    +
    + {{ radios( + form.sender, + option_hints=option_hints, + hide_legend=True + ) }} + {{ page_footer( + 'Continue', + back_link=url_for('.view_template', service_id=current_service.id, template_id=template_id), + back_link_text='Back to template' + ) }} + {% if no_senders %} + Add new sender + {% endif %} +
    +
    +
    + +{% endblock %} diff --git a/app/utils.py b/app/utils.py index ba19eddc7..bbeebb691 100644 --- a/app/utils.py +++ b/app/utils.py @@ -300,11 +300,12 @@ def get_template( template, image_url=letter_preview_url, page_count=int(page_count), + contact_block=template['reply_to_text'] ) else: return LetterPreviewTemplate( template, - contact_block=service['letter_contact_block'], + contact_block=template['reply_to_text'], admin_base_url=current_app.config['ADMIN_BASE_URL'], redact_missing_personalisation=redact_missing_personalisation, ) diff --git a/tests/__init__.py b/tests/__init__.py index b5ba0350d..6fef78218 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -100,6 +100,9 @@ def template_json(service_id, archived=False, process_type='normal', redact_personalisation=None, + service_letter_contact=None, + reply_to=None, + reply_to_text=None, ): template = { 'id': id_, @@ -111,6 +114,9 @@ def template_json(service_id, 'updated_at': datetime.utcnow().strftime('%Y-%m-%d %H:%M:%S.%f'), 'archived': archived, 'process_type': process_type, + 'service_letter_contact': service_letter_contact, + 'reply_to': reply_to, + 'reply_to_text': reply_to_text, } if content is None: template['content'] = "template content" @@ -236,6 +242,7 @@ def notification_json( rows=5, personalisation=None, template_type=None, + reply_to_text=None ): if template is None: template = template_json(service_id, str(generate_uuid()), type_=template_type) @@ -283,6 +290,7 @@ def notification_json( 'template_version': template['version'], 'personalisation': personalisation or {}, 'notification_type': template_type, + 'reply_to_text': reply_to_text, } for i in range(rows)], 'total': rows, 'page_size': 50, diff --git a/tests/app/main/views/test_api_integration.py b/tests/app/main/views/test_api_integration.py new file mode 100644 index 000000000..aa2ec3e46 --- /dev/null +++ b/tests/app/main/views/test_api_integration.py @@ -0,0 +1,639 @@ +import uuid +from collections import OrderedDict + +import pytest +from flask import url_for +from bs4 import BeautifulSoup +from unittest.mock import call + +from tests import validate_route_permission +from tests.conftest import ( + mock_get_service, + mock_get_live_service, + mock_get_service_with_letters, + normalize_spaces, + SERVICE_ONE_ID, + mock_get_valid_service_callback_api, + mock_get_valid_service_inbound_api, +) + + +def test_should_show_api_page( + logged_in_client, + mock_login, + api_user_active, + mock_get_service, + mock_has_permissions, + mock_get_notifications +): + response = logged_in_client.get(url_for('main.api_integration', service_id=str(uuid.uuid4()))) + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'API integration' + rows = page.find_all('details') + assert len(rows) == 5 + for index, row in enumerate(rows): + assert row.find('h3').string.strip() == '07123456789' + + +def test_should_show_api_page_with_lots_of_notifications( + logged_in_client, + mock_login, + api_user_active, + mock_get_service, + mock_has_permissions, + mock_get_notifications_with_previous_next +): + response = logged_in_client.get(url_for('main.api_integration', service_id=str(uuid.uuid4()))) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + rows = page.find_all('div', {'class': 'api-notifications-item'}) + assert ' '.join(rows[len(rows) - 1].text.split()) == ( + 'Only showing the first 50 messages. Notify deletes messages after 7 days.' + ) + + +def test_should_show_api_page_with_no_notifications( + logged_in_client, + mock_login, + api_user_active, + mock_get_service, + mock_has_permissions, + mock_get_notifications_with_no_notifications +): + response = logged_in_client.get(url_for('main.api_integration', service_id=str(uuid.uuid4()))) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + rows = page.find_all('div', {'class': 'api-notifications-item'}) + assert 'When you send messages via the API they’ll appear here.' in rows[len(rows) - 1].text.strip() + + +def test_should_show_api_page_for_live_service( + logged_in_client, + mock_login, + api_user_active, + mock_get_live_service, + mock_has_permissions +): + response = logged_in_client.get(url_for('main.api_integration', service_id=str(uuid.uuid4()))) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert 'Your service is in trial mode' not in page.find('main').text + + +def test_api_documentation_page_should_redirect( + logged_in_client, + mock_login, + api_user_active, + mock_get_service, + mock_has_permissions +): + response = logged_in_client.get(url_for('main.api_documentation', service_id=str(uuid.uuid4()))) + assert response.status_code == 301 + assert response.location == url_for( + 'main.documentation', + _external=True + ) + + +def test_should_show_empty_api_keys_page( + client, + api_user_pending, + mock_login, + mock_get_no_api_keys, + mock_get_service, + mock_has_permissions, +): + client.login(api_user_pending) + service_id = str(uuid.uuid4()) + response = client.get(url_for('main.api_keys', service_id=service_id)) + + assert response.status_code == 200 + assert 'You haven’t created any API keys yet' in response.get_data(as_text=True) + assert 'Create an API key' in response.get_data(as_text=True) + mock_get_no_api_keys.assert_called_once_with(service_id=service_id) + + +def test_should_show_api_keys_page( + logged_in_client, + api_user_active, + mock_login, + mock_get_api_keys, + mock_get_service, + mock_has_permissions, + fake_uuid, +): + response = logged_in_client.get(url_for('main.api_keys', service_id=fake_uuid)) + + assert response.status_code == 200 + resp_data = response.get_data(as_text=True) + assert 'some key name' in resp_data + assert 'another key name' in resp_data + assert 'Revoked 1 January at 1:00am' in resp_data + mock_get_api_keys.assert_called_once_with(service_id=fake_uuid) + + +@pytest.mark.parametrize('service_mock, expected_options', [ + (mock_get_service, [ + ( + 'Live – sends to anyone ' + 'Not available because your service is in trial mode' + ), + 'Team and whitelist – limits who you can send to', + 'Test – pretends to send messages', + ]), + (mock_get_live_service, [ + 'Live – sends to anyone', + 'Team and whitelist – limits who you can send to', + 'Test – pretends to send messages', + ]), + (mock_get_service_with_letters, [ + 'Live – sends to anyone', + ( + 'Team and whitelist – limits who you can send to ' + 'Can’t be used to send letters' + ), + 'Test – pretends to send messages', + ]), +]) +def test_should_show_create_api_key_page( + client_request, + mocker, + api_user_active, + mock_get_api_keys, + service_mock, + expected_options, +): + service_mock(mocker, api_user_active) + + page = client_request.get('main.create_api_key', service_id=SERVICE_ONE_ID) + + for index, option in enumerate(expected_options): + assert normalize_spaces(page.select('.block-label')[index].text) == option + + +def test_should_create_api_key_with_type_normal( + logged_in_client, + api_user_active, + mock_login, + mock_get_api_keys, + mock_get_live_service, + mock_has_permissions, + fake_uuid, + mocker, +): + post = mocker.patch('app.notify_client.api_key_api_client.ApiKeyApiClient.post', return_value={'data': fake_uuid}) + service_id = str(uuid.uuid4()) + + response = logged_in_client.post( + url_for('main.create_api_key', service_id=service_id), + data={ + 'key_name': 'Some default key name 1/2', + 'key_type': 'normal' + } + ) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + keys = page.find_all('span', {'class': 'api-key-key'}) + for index, key in enumerate([ + 'some_default_key_name_12-{}-{}'.format(service_id, fake_uuid), + service_id, + fake_uuid + ]): + assert keys[index].text.strip() == key + + post.assert_called_once_with(url='/service/{}/api-key'.format(service_id), data={ + 'name': 'Some default key name 1/2', + 'key_type': 'normal', + 'created_by': api_user_active.id + }) + + +def test_cant_create_normal_api_key_in_trial_mode( + logged_in_client, + api_user_active, + mock_login, + mock_get_api_keys, + mock_get_service, + mock_has_permissions, + fake_uuid, + mocker, +): + mock_post = mocker.patch('app.notify_client.api_key_api_client.ApiKeyApiClient.post') + + response = logged_in_client.post( + url_for('main.create_api_key', service_id=uuid.uuid4()), + data={ + 'key_name': 'some default key name', + 'key_type': 'normal' + } + ) + assert response.status_code == 400 + mock_post.assert_not_called() + + +def test_should_show_confirm_revoke_api_key( + client_request, + mock_get_api_keys, + fake_uuid, +): + page = client_request.get( + 'main.revoke_api_key', service_id=SERVICE_ONE_ID, key_id=fake_uuid, + _test_page_title=False, + ) + assert normalize_spaces(page.select('.banner-dangerous')[0].text) == ( + 'Are you sure you want to revoke this API key? ' + '‘some key name’ will no longer let you connect to GOV.UK Notify.' + ) + assert mock_get_api_keys.call_args_list == [ + call( + key_id=fake_uuid, + service_id='596364a0-858e-42c8-9062-a8fe822260eb', + ), + call( + service_id='596364a0-858e-42c8-9062-a8fe822260eb' + ), + ] + + +def test_should_redirect_after_revoking_api_key( + logged_in_client, + api_user_active, + mock_login, + mock_revoke_api_key, + mock_get_api_keys, + mock_get_service, + mock_has_permissions, + fake_uuid, +): + response = logged_in_client.post(url_for('main.revoke_api_key', service_id=fake_uuid, key_id=fake_uuid)) + + assert response.status_code == 302 + assert response.location == url_for('.api_keys', service_id=fake_uuid, _external=True) + mock_revoke_api_key.assert_called_once_with(service_id=fake_uuid, key_id=fake_uuid) + mock_get_api_keys.assert_called_once_with(service_id=fake_uuid, key_id=fake_uuid) + + +@pytest.mark.parametrize('route', [ + 'main.api_keys', + 'main.create_api_key', + 'main.revoke_api_key' +]) +def test_route_permissions( + mocker, + app_, + api_user_active, + service_one, + mock_get_api_keys, + route, +): + with app_.test_request_context(): + validate_route_permission( + mocker, + app_, + "GET", + 200, + url_for(route, service_id=service_one['id'], key_id=123), + ['manage_api_keys'], + api_user_active, + service_one) + + +@pytest.mark.parametrize('route', [ + 'main.api_keys', + 'main.create_api_key', + 'main.revoke_api_key' +]) +def test_route_invalid_permissions( + mocker, + app_, + api_user_active, + service_one, + mock_get_api_keys, + route, +): + with app_.test_request_context(): + validate_route_permission( + mocker, + app_, + "GET", + 403, + url_for(route, service_id=service_one['id'], key_id=123), + ['view_activity'], + api_user_active, + service_one) + + +def test_should_show_whitelist_page( + logged_in_client, + mock_login, + api_user_active, + mock_get_service, + mock_has_permissions, + mock_get_whitelist, +): + response = logged_in_client.get(url_for('main.whitelist', service_id=str(uuid.uuid4()))) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + textboxes = page.find_all('input', {'type': 'text'}) + for index, value in enumerate( + ['test@example.com'] + [''] * 4 + ['07900900000'] + [''] * 4 + ): + assert textboxes[index]['value'] == value + + +def test_should_update_whitelist( + logged_in_client, + mock_login, + api_user_active, + mock_get_service, + mock_has_permissions, + mock_update_whitelist +): + service_id = str(uuid.uuid4()) + data = OrderedDict([ + ('email_addresses-1', 'test@example.com'), + ('email_addresses-3', 'test@example.com'), + ('phone_numbers-0', '07900900000'), + ('phone_numbers-2', '+1800-555-555'), + ]) + + logged_in_client.post( + url_for('main.whitelist', service_id=service_id), + data=data + ) + + mock_update_whitelist.assert_called_once_with(service_id, { + 'email_addresses': ['test@example.com', 'test@example.com'], + 'phone_numbers': ['07900900000', '+1800-555-555']}) + + +def test_should_validate_whitelist_items( + logged_in_client, + mock_login, + api_user_active, + mock_get_service, + mock_has_permissions, + mock_update_whitelist +): + + response = logged_in_client.post( + url_for('main.whitelist', service_id=str(uuid.uuid4())), + data=OrderedDict([ + ('email_addresses-1', 'abc'), + ('phone_numbers-0', '123') + ]) + ) + + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'There was a problem with your whitelist' + jump_links = page.select('.banner-dangerous a') + + assert jump_links[0].string.strip() == 'Enter valid email addresses' + assert jump_links[0]['href'] == '#email_addresses' + + assert jump_links[1].string.strip() == 'Enter valid phone numbers' + assert jump_links[1]['href'] == '#phone_numbers' + + mock_update_whitelist.assert_not_called() + + +@pytest.mark.parametrize('endpoint', [ + ('main.delivery_status_callback'), + ('main.received_text_messages_callback'), +]) +@pytest.mark.parametrize('url, bearer_token, expected_errors', [ + ("", "", "Can’t be empty Can’t be empty"), + ("http://not_https.com", "1234567890", "Must be a valid https URL"), + ("https://test.com", "123456789", "Must be at least 10 characters"), +]) +def test_callback_forms_validation( + client_request, + service_one, + endpoint, + url, + bearer_token, + expected_errors +): + if endpoint == 'main.received_text_messages_callback': + service_one['permissions'] = ['inbound_sms'] + + data = { + "url": url, + "bearer_token": bearer_token, + } + + response = client_request.post( + endpoint, + service_id=service_one['id'], + _data=data, + _expected_status=200 + ) + error_msgs = ' '.join(msg.text.strip() for msg in response.select(".error-message")) + + assert error_msgs == expected_errors + + +@pytest.mark.parametrize('has_inbound_sms, expected_link', [ + (True, 'main.api_callbacks'), + (False, 'main.delivery_status_callback'), +]) +def test_callbacks_button_links_straight_to_delivery_status_if_service_has_no_inbound_sms( + client_request, + service_one, + mocker, + mock_get_notifications, + has_inbound_sms, + expected_link +): + if has_inbound_sms: + service_one['permissions'] = ['inbound_sms'] + + page = client_request.get( + 'main.api_integration', + service_id=service_one['id'], + ) + + assert page.select('.pill-separate-item')[2]['href'] == url_for( + expected_link, service_id=service_one['id'] + ) + + +def test_callbacks_page_redirects_to_delivery_status_if_service_has_no_inbound_sms( + client_request, + service_one, + mocker +): + page = client_request.get( + 'main.api_callbacks', + service_id=service_one['id'], + _follow_redirects=True, + ) + + assert normalize_spaces(page.select_one('h1').text) == "Callbacks for delivery receipts" + + +@pytest.mark.parametrize('has_inbound_sms, expected_link', [ + (True, 'main.api_callbacks'), + (False, 'main.api_integration'), +]) +def test_back_link_directs_to_api_integration_from_delivery_callback_if_no_inbound_sms( + client_request, + service_one, + mocker, + has_inbound_sms, + expected_link +): + if has_inbound_sms: + service_one['permissions'] = ['inbound_sms'] + + page = client_request.get( + 'main.delivery_status_callback', + service_id=service_one['id'], + _follow_redirects=True, + ) + + assert page.select_one('.page-footer-back-link')['href'] == url_for( + expected_link, service_id=service_one['id'] + ) + + +@pytest.mark.parametrize('endpoint', [ + ('main.delivery_status_callback'), + ('main.received_text_messages_callback'), +]) +def test_create_delivery_status_and_receive_text_message_callbacks( + client_request, + service_one, + mocker, + mock_get_notifications, + mock_create_service_inbound_api, + mock_create_service_callback_api, + endpoint, + fake_uuid, +): + if endpoint == 'main.received_text_messages_callback': + service_one['permissions'] = ['inbound_sms'] + + data = { + 'url': "https://test.url.com/", + 'bearer_token': '1234567890', + 'user_id': fake_uuid + } + + client_request.post( + endpoint, + service_id=service_one['id'], + _data=data, + ) + + if endpoint == 'main.received_text_messages_callback': + mock_create_service_inbound_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid, + ) + else: + mock_create_service_callback_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid, + ) + + +@pytest.mark.parametrize('endpoint, fixture', [ + ('main.delivery_status_callback', mock_get_valid_service_callback_api), + ('main.received_text_messages_callback', mock_get_valid_service_inbound_api), +]) +def test_update_delivery_status_and_receive_text_message_callbacks( + client_request, + service_one, + mocker, + mock_get_notifications, + mock_update_service_inbound_api, + mock_update_service_callback_api, + endpoint, + fixture, + fake_uuid, +): + if endpoint == 'main.received_text_messages_callback': + service_one['inbound_api'] = [fake_uuid] + service_one['permissions'] = ['inbound_sms'] + else: + service_one['service_callback_api'] = [fake_uuid] + + fixture(mocker) + + data = { + 'url': "https://test.url.com/", + 'bearer_token': '1234567890', + 'user_id': fake_uuid + } + + client_request.post( + endpoint, + service_id=service_one['id'], + _data=data, + ) + + if endpoint == 'main.received_text_messages_callback': + mock_update_service_inbound_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid, + inbound_api_id=fake_uuid, + ) + else: + mock_update_service_callback_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid, + callback_api_id=fake_uuid + ) + + +@pytest.mark.parametrize('endpoint, data, fixture', [ + ( + 'main.delivery_status_callback', + {"url": "https://hello2.gov.uk", "bearer_token": "bearer_token_set"}, + mock_get_valid_service_callback_api + ), + ( + 'main.received_text_messages_callback', + {"url": "https://hello3.gov.uk", "bearer_token": "bearer_token_set"}, + mock_get_valid_service_inbound_api + ), +]) +def test_update_delivery_status_and_receive_text_message_callbacks_without_changes_do_not_update( + client_request, + service_one, + mocker, + mock_get_notifications, + mock_update_service_callback_api, + mock_update_service_inbound_api, + data, + fixture, + endpoint, + fake_uuid, +): + if endpoint == 'main.received_text_messages_callback': + service_one['inbound_api'] = [fake_uuid] + service_one['permissions'] = ['inbound_sms'] + else: + service_one['service_callback_api'] = [fake_uuid] + + fixture(mocker) + + data['user_id'] = fake_uuid + + client_request.post( + endpoint, + service_id=service_one['id'], + _data=data, + ) + + if endpoint == 'main.received_text_messages_callback': + assert mock_update_service_inbound_api.called is False + else: + assert mock_update_service_callback_api.called is False diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 02d22db84..fb8cb7d76 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -908,6 +908,34 @@ def test_add_letter_contact( ) +def test_add_letter_contact_when_coming_from_template( + no_letter_contact_blocks, + client_request, + mock_add_letter_contact, + fake_uuid, + mock_get_service_letter_template, +): + data = { + 'letter_contact_block': "1 Example Street" + } + + page = client_request.post( + 'main.service_add_letter_contact', + service_id=SERVICE_ONE_ID, + _data=data, + from_template=fake_uuid, + _follow_redirects=True + ) + + mock_add_letter_contact.assert_called_once_with( + SERVICE_ONE_ID, + contact_block="1 Example Street", + is_default=True + ) + + assert page.find('h1').text == 'Set letter contact block' + + @pytest.mark.parametrize('fixture, data, api_default_args', [ (no_sms_senders, {}, True), (multiple_sms_senders, {}, False), diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index eb5ae48e0..d33982af2 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -11,6 +11,8 @@ from tests.conftest import ( mock_get_service_email_template, mock_get_service_letter_template, mock_get_service_template, + no_letter_contact_blocks, + single_letter_contact_block, normalize_spaces, SERVICE_ONE_ID, active_user_with_permissions, @@ -198,31 +200,7 @@ def test_should_show_sms_template_with_downgraded_unicode_characters( assert rendered_msg in response.get_data(as_text=True) -def test_should_let_letter_contact_block_be_edited_if_a_letter_contact_block_exists( - mocker, - mock_get_service_letter_template, - single_letter_contact_block, - client_request, - service_one, - fake_uuid, -): - service_one['permissions'].append('letter') - mocker.patch('app.main.views.templates.get_page_count_for_letter', return_value=1) - - page = client_request.get( - 'main.view_template', - service_id=SERVICE_ONE_ID, - template_id=fake_uuid - ) - - assert page.find('a', {'class': 'edit-template-link-letter-contact'})['href'] == url_for( - '.service_edit_letter_contact', - service_id=service_one['id'], - letter_contact_id='1234', - from_template=fake_uuid) - - -def test_should_let_letter_contact_block_be_added_if_no_letter_contact_blocks_exist( +def test_should_let_letter_contact_block_be_changed_for_the_template( mocker, mock_get_service_letter_template, no_letter_contact_blocks, @@ -240,9 +218,10 @@ def test_should_let_letter_contact_block_be_added_if_no_letter_contact_blocks_ex ) assert page.find('a', {'class': 'edit-template-link-letter-contact'})['href'] == url_for( - '.service_add_letter_contact', - service_id=service_one['id'], - from_template=fake_uuid) + 'main.set_template_sender', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + ) def test_should_show_page_template_with_priority_select_if_platform_admin( @@ -1286,3 +1265,51 @@ def test_should_show_letter_template_as_dvla_markup( )) assert response.status_code == expected_response_code + + +def test_set_template_sender( + client_request, + fake_uuid, + mock_update_service_template_sender, + mock_get_service_letter_template, + single_letter_contact_block +): + data = { + 'sender': '1234', + } + + client_request.post( + 'main.set_template_sender', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _data=data, + ) + + mock_update_service_template_sender.assert_called_once_with( + SERVICE_ONE_ID, + fake_uuid, + '1234', + ) + + +@pytest.mark.parametrize('fixture, add_button_is_on_page', [ + (no_letter_contact_blocks, True), + (single_letter_contact_block, False), +]) +def test_add_sender_link_only_appears_on_services_with_no_senders( + client_request, + fake_uuid, + mocker, + fixture, + add_button_is_on_page, + mock_get_service_letter_template, + no_letter_contact_blocks +): + fixture(mocker) + page = client_request.get( + 'main.set_template_sender', + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + ) + + assert (page.select_one('.column-three-quarters form > a') is not None) == add_button_is_on_page diff --git a/tests/app/test_template_previews.py b/tests/app/test_template_previews.py index e1088bbdf..933371a95 100644 --- a/tests/app/test_template_previews.py +++ b/tests/app/test_template_previews.py @@ -45,20 +45,22 @@ def test_from_database_object_makes_request( client, partial_call, expected_url, + mock_get_service_letter_template ): resp = Mock(content='a', status_code='b', headers={'c': 'd'}) request_mock = mocker.patch('app.template_previews.requests.post', return_value=resp) mocker.patch('app.template_previews.current_service', __getitem__=Mock(return_value='123')) + template = mock_get_service_letter_template('123', '456')['data'] - ret = partial_call(template='foo') + ret = partial_call(template=template) assert ret[0] == 'a' assert ret[1] == 'b' assert list(ret[2]) == [('c', 'd')] data = { - 'letter_contact_block': '123', - 'template': 'foo', + 'letter_contact_block': None, + 'template': template, 'values': None, 'dvla_org_id': '123', } diff --git a/tests/conftest.py b/tests/conftest.py index 3e9c5c466..b6bd4030b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1029,6 +1029,17 @@ def mock_redact_template(mocker): return mocker.patch('app.service_api_client.redact_service_template') +@pytest.fixture(scope='function') +def mock_update_service_template_sender(mocker): + def _update(service_id, template_id, reply_to): + return + + return mocker.patch( + 'app.service_api_client.update_service_template_sender', + side_effect=_update + ) + + @pytest.fixture(scope='function') def api_user_pending(fake_uuid): from app.notify_client.user_api_client import User