diff --git a/Makefile b/Makefile index d0a559275..6884c678b 100644 --- a/Makefile +++ b/Makefile @@ -70,9 +70,17 @@ build-codedeploy-artifact: ## Build the deploy artifact for CodeDeploy mkdir -p target zip -r -x@deploy-exclude.lst target/notifications-api.zip * + rm -rf build/db-migration-codedeploy + mkdir -p build/db-migration-codedeploy + unzip target/notifications-api.zip -d build/db-migration-codedeploy + cd build/db-migration-codedeploy && \ + mv -f appspec-db-migration.yml appspec.yml && \ + zip -r -x@deploy-exclude.lst ../../target/notifications-api-db-migration.zip * + .PHONY: upload-codedeploy-artifact ## Upload the deploy artifact for CodeDeploy upload-codedeploy-artifact: check-env-vars aws s3 cp --region eu-west-1 target/notifications-api.zip s3://${DNS_NAME}-codedeploy/notifications-api-${DEPLOY_BUILD_NUMBER}.zip + aws s3 cp --region eu-west-1 target/notifications-api-db-migration.zip s3://${DNS_NAME}-codedeploy/notifications-api-db-migration-${DEPLOY_BUILD_NUMBER}.zip .PHONY: test test: venv generate-version-file ## Run tests @@ -82,6 +90,10 @@ test: venv generate-version-file ## Run tests deploy-api: check-env-vars ## Trigger CodeDeploy for the api aws deploy create-deployment --application-name notify-api --deployment-config-name CodeDeployDefault.OneAtATime --deployment-group-name notify-api --s3-location bucket=${DNS_NAME}-codedeploy,key=notifications-api-${DEPLOY_BUILD_NUMBER}.zip,bundleType=zip --region eu-west-1 +.PHONY: deploy-api +deploy-api-db-migration: check-env-vars ## Trigger CodeDeploy for the api db migration + aws deploy create-deployment --application-name notify-api-db-migration --deployment-config-name CodeDeployDefault.OneAtATime --deployment-group-name notify-api-db-migration --s3-location bucket=${DNS_NAME}-codedeploy,key=notifications-api-db-migration-${DEPLOY_BUILD_NUMBER}.zip,bundleType=zip --region eu-west-1 + .PHONY: deploy-admin-api deploy-admin-api: check-env-vars ## Trigger CodeDeploy for the admin api aws deploy create-deployment --application-name notify-admin-api --deployment-config-name CodeDeployDefault.OneAtATime --deployment-group-name notify-admin-api --s3-location bucket=${DNS_NAME}-codedeploy,key=notifications-api-${DEPLOY_BUILD_NUMBER}.zip,bundleType=zip --region eu-west-1 @@ -155,4 +167,4 @@ clean-docker-containers: ## Clean up any remaining docker containers docker rm -f $(shell docker ps -q -f "name=${DOCKER_CONTAINER_PREFIX}") 2> /dev/null || true clean: - rm -rf node_modules cache target venv .coverage + rm -rf node_modules cache target venv .coverage build tests/.cache diff --git a/app/dao/provider_statistics_dao.py b/app/dao/provider_statistics_dao.py index 9a29cffe0..588e9a7dc 100644 --- a/app/dao/provider_statistics_dao.py +++ b/app/dao/provider_statistics_dao.py @@ -1,9 +1,7 @@ -from sqlalchemy import func, cast, Float, case +from sqlalchemy import func from app import db from app.models import ( - ProviderStatistics, - ProviderDetails, NotificationHistory, SMS_TYPE, EMAIL_TYPE, @@ -12,15 +10,6 @@ from app.models import ( ) -def get_provider_statistics(service, **kwargs): - query = ProviderStatistics.query.filter_by(service=service) - if 'providers' in kwargs: - providers = ProviderDetails.query.filter(ProviderDetails.identifier.in_(kwargs['providers'])).all() - provider_ids = [provider.id for provider in providers] - query = query.filter(ProviderStatistics.provider_id.in_(provider_ids)) - return query - - def get_fragment_count(service_id): shared_filters = [ NotificationHistory.service_id == service_id, diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 60bca95c4..ca36a43a5 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -79,6 +79,7 @@ def send_email_to_provider(notification): send_email_response.apply_async( (provider.get_name(), reference, notification.to), queue='research-mode' ) + notification.billable_units = 0 else: from_address = '"{}" <{}@{}>'.format(service.name, service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']) diff --git a/app/service/rest.py b/app/service/rest.py index a6adebdd2..011977a84 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -213,6 +213,7 @@ def get_all_notifications_for_service(service_id): page = data['page'] if 'page' in data else 1 page_size = data['page_size'] if 'page_size' in data else current_app.config.get('PAGE_SIZE') limit_days = data.get('limit_days') + include_jobs = data.get('include_jobs', True) pagination = notifications_dao.get_notifications_for_service( service_id, @@ -220,7 +221,7 @@ def get_all_notifications_for_service(service_id): page=page, page_size=page_size, limit_days=limit_days, - include_jobs=True) + include_jobs=include_jobs) kwargs = request.args.to_dict() kwargs['service_id'] = service_id return jsonify( diff --git a/appspec-db-migration.yml b/appspec-db-migration.yml new file mode 100644 index 000000000..b840f4432 --- /dev/null +++ b/appspec-db-migration.yml @@ -0,0 +1,18 @@ +--- +os: linux +version: 0.0 +files: + - destination: /home/notify-app/notifications-api + source: / +hooks: + AfterInstall: + - location: scripts/aws_install_dependencies.sh + runas: root + timeout: 1000 + - location: scripts/aws_change_ownership.sh + runas: root + timeout: 300 + ApplicationStart: + - location: scripts/aws_run_db_migrations.sh + runas: root + timeout: 300 diff --git a/appspec.yml b/appspec.yml index 77f7ed385..a6455d55c 100644 --- a/appspec.yml +++ b/appspec.yml @@ -1,35 +1,28 @@ --- +os: linux +version: 0.0 files: - - - destination: /home/notify-app/notifications-api + - destination: /home/notify-app/notifications-api source: / hooks: AfterInstall: - - - location: scripts/aws_install_dependencies.sh + - location: scripts/aws_install_dependencies.sh runas: root timeout: 1000 - - - location: scripts/aws_change_ownership.sh + - location: scripts/aws_change_ownership.sh runas: root timeout: 300 ApplicationStart: - - - location: scripts/aws_start_app.sh + - location: scripts/aws_start_app.sh runas: root timeout: 300 - - - location: scripts/register_with_elb.sh + - location: scripts/register_with_elb.sh runas: ubuntu timeout: 300 ApplicationStop: - - - location: scripts/deregister_from_elb.sh + - location: scripts/deregister_from_elb.sh runas: ubuntu timeout: 300 - - - location: scripts/aws_stop_app.sh + - location: scripts/aws_stop_app.sh runas: root timeout: 300 -os: linux -version: 0.0 diff --git a/deploy-exclude.lst b/deploy-exclude.lst index 5eaf61748..aea0bc983 100644 --- a/deploy-exclude.lst +++ b/deploy-exclude.lst @@ -7,3 +7,6 @@ *node_modules* *target* *venv* +*build* +*.envrc* +*tests/.cache* diff --git a/notifications-api.zip b/notifications-api.zip deleted file mode 100644 index 83298c812..000000000 Binary files a/notifications-api.zip and /dev/null differ diff --git a/scripts/aws_change_ownership.sh b/scripts/aws_change_ownership.sh index 09d3d80cf..ad0d438a7 100755 --- a/scripts/aws_change_ownership.sh +++ b/scripts/aws_change_ownership.sh @@ -1,13 +1,8 @@ #!/bin/bash +set -eo pipefail -if [ -e "/home/notify-app" ] -then - echo "Chown application to be owned by notify-app" - cd /home/notify-app/; - chown -R notify-app:govuk-notify-applications notifications-api -else - echo "Chown application to be owned by ubuntu" - cd /home/ubuntu/; - chown -R ubuntu:ubuntu notifications-api -fi \ No newline at end of file +echo "Chown application to be owned by notify-app" + +cd /home/notify-app/; +chown -R notify-app:govuk-notify-applications notifications-api diff --git a/scripts/aws_install_dependencies.sh b/scripts/aws_install_dependencies.sh index f7cb975f6..b82881d45 100755 --- a/scripts/aws_install_dependencies.sh +++ b/scripts/aws_install_dependencies.sh @@ -1,15 +1,8 @@ +#!/bin/bash + +set -eo pipefail + echo "Install dependencies" - -if [ -e "/home/notify-app" ] -then - echo "Depenencies for notify-app" - cd /home/notify-app/notifications-api; - pip3 install -r /home/notify-app/notifications-api/requirements.txt - python3 db.py db upgrade -else - echo "Depenencies for ubuntu" - cd /home/ubuntu/notifications-api; - pip3 install -r /home/ubuntu/notifications-api/requirements.txt - python3 db.py db upgrade -fi \ No newline at end of file +cd /home/notify-app/notifications-api; +pip3 install -r /home/notify-app/notifications-api/requirements.txt diff --git a/scripts/aws_run_db_migrations.sh b/scripts/aws_run_db_migrations.sh new file mode 100755 index 000000000..9667839d5 --- /dev/null +++ b/scripts/aws_run_db_migrations.sh @@ -0,0 +1,8 @@ +#!/usr/bin/env bash + +set -eo pipefail + +echo "Run database migrations" + +cd /home/notify-app/notifications-api; +python3 db.py db upgrade diff --git a/scripts/aws_start_app.sh b/scripts/aws_start_app.sh index 47f7103ba..9e871ebc2 100755 --- a/scripts/aws_start_app.sh +++ b/scripts/aws_start_app.sh @@ -1,31 +1,19 @@ -#!/bin/bash +#!/usr/bin/env bash -if [ -e "/etc/init/notifications-api.conf" ] -then - echo "Starting api" - sudo service notifications-api start -fi +set -eo pipefail -if [ -e "/etc/init/notifications-api-celery-worker.conf" ] -then - echo "Starting celery worker" - sudo service notifications-api-celery-worker start -fi +function start +{ + service=$1 + if [ -e "/etc/init/${service}.conf" ] + then + echo "Starting ${service}" + service ${service} start + fi +} -if [ -e "/etc/init/notifications-api-celery-worker-sender.conf" ] -then - echo "Starting celery worker" - sudo service notifications-api-celery-worker-sender start -fi - -if [ -e "/etc/init/notifications-api-celery-worker-db.conf" ] -then - echo "Starting celery worker" - sudo service notifications-api-celery-worker-db start -fi - -if [ -e "/etc/init/notifications-api-celery-beat.conf" ] -then - echo "Starting celery beat" - sudo service notifications-api-celery-beat start -fi +start "notifications-api" +start "notifications-api-celery-worker" +start "notifications-api-celery-worker-sender" +start "notifications-api-celery-worker-db" +start "notifications-api-celery-beat" diff --git a/scripts/aws_stop_app.sh b/scripts/aws_stop_app.sh index 3f9735d4b..f3c3a441a 100755 --- a/scripts/aws_stop_app.sh +++ b/scripts/aws_stop_app.sh @@ -1,53 +1,22 @@ -#!/bin/bash +#!/usr/bin/env bash +set -eo pipefail -function error_exit +function stop { - echo "$1" 1>&2 - exit 0 + service=$1 + if [ -e "/etc/init/${service}.conf" ]; then + echo "stopping ${service}" + if service ${service} stop; then + echo "${service} stopped" + else + >&2 echo "Could not stop ${service}" + fi + fi } -if [ -e "/etc/init/notifications-api.conf" ]; then - echo "stopping notifications-api" - if sudo service notifications-api stop; then - echo "notifications-api stopped" - else - error_exit "Could not stop notifications-api" - fi -fi - -if [ -e "/etc/init/notifications-api-celery-beat.conf" ]; then - echo "stopping notifications-api-celery-beat" - if sudo service notifications-api-celery-beat stop; then - echo "notifications-api beat stopped" - else - error_exit "Could not stop notifications-celery-beat" - fi -fi - -if [ -e "/etc/init/notifications-api-celery-worker.conf" ]; then - echo "stopping notifications-api-celery-worker" - if sudo service notifications-api-celery-worker stop; then - echo "notifications-api worker stopped" - else - error_exit "Could not stop notifications-celery-worker" - fi -fi - -if [ -e "/etc/init/notifications-api-celery-worker-sender.conf" ]; then - echo "stopping notifications-api-celery-worker-sender" - if sudo service notifications-api-celery-worker-sender stop; then - echo "notifications-api sender worker stopped" - else - error_exit "Could not stop notifications-celery-worker-sender" - fi -fi - -if [ -e "/etc/init/notifications-api-celery-worker-db.conf" ]; then - echo "stopping notifications-api-celery-worker-db" - if sudo service notifications-api-celery-worker-db stop; then - echo "notifications-api db worker stopped" - else - error_exit "Could not stop notifications-celery-worker-db" - fi -fi +stop "notifications-api" +stop "notifications-api-celery-beat" +stop "notifications-api-celery-worker" +stop "notifications-api-celery-worker-sender" +stop "notifications-api-celery-worker-db" diff --git a/setup.cfg b/setup.cfg index c19f0a88a..1bb52e772 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,4 +1,4 @@ [pep8] max-line-length = 120 ignore = E402 -exclude = ./migrations,./venv,./venv3 +exclude = ./migrations,./venv,./venv3,./build diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index b2f7f28e7..a2af75d36 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -142,7 +142,9 @@ def test_should_add_to_retry_queue_if_notification_not_found_in_send_email_to_pr app.celery.provider_tasks.send_email_to_provider.retry.assert_called_with(queue="retry", countdown=10) -def test_should_go_into_technical_error_if_exceeds_retries( +# DO THESE FOR THE 4 TYPES OF TASK + +def test_should_go_into_technical_error_if_exceeds_retries_on_send_sms_to_provider_task( notify_db, notify_db_session, sample_service, @@ -164,7 +166,28 @@ def test_should_go_into_technical_error_if_exceeds_retries( assert db_notification.status == 'technical-failure' -def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retries( +def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task( + notify_db, + notify_db_session, + sample_service, + mocker): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + service=sample_service, status='created') + + mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=Exception("EXPECTED")) + mocker.patch('app.celery.provider_tasks.deliver_sms.retry', side_effect=MaxRetriesExceededError()) + + deliver_sms( + notification.id + ) + + provider_tasks.deliver_sms.retry.assert_called_with(queue='retry', countdown=10) + + db_notification = Notification.query.filter_by(id=notification.id).one() + assert db_notification.status == 'technical-failure' + + +def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retries_on_send_email_to_provider_task( notify_db, notify_db_session, sample_service, @@ -185,3 +208,24 @@ def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retrie db_notification = Notification.query.filter_by(id=notification.id).one() assert db_notification.status == 'technical-failure' + + +def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task( + notify_db, + notify_db_session, + sample_service, + mocker): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + service=sample_service, status='created') + + mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=Exception("EXPECTED")) + mocker.patch('app.celery.provider_tasks.deliver_email.retry', side_effect=MaxRetriesExceededError()) + + deliver_email( + notification.id + ) + + provider_tasks.deliver_email.retry.assert_called_with(queue='retry', countdown=10) + + db_notification = Notification.query.filter_by(id=notification.id).one() + assert db_notification.status == 'technical-failure' diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 94e0ff6f5..3f91d36fc 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -7,10 +7,10 @@ from mock import ANY import app from sqlalchemy.orm.exc import NoResultFound from app import mmg_client -from app.dao import (provider_details_dao, notifications_dao, provider_statistics_dao) -from app.dao.provider_statistics_dao import get_provider_statistics +from app.dao import (provider_details_dao, notifications_dao) from app.delivery import send_to_providers -from app.models import Notification, KEY_TYPE_NORMAL, KEY_TYPE_TEST, BRANDING_ORG, BRANDING_BOTH, Organisation +from app.models import Notification, KEY_TYPE_NORMAL, KEY_TYPE_TEST, BRANDING_ORG, BRANDING_BOTH, Organisation, \ + KEY_TYPE_TEAM from tests.app.conftest import sample_notification from notifications_utils.recipients import validate_phone_number, format_phone_number @@ -197,8 +197,6 @@ def test_should_call_send_sms_response_task_if_research_mode(notify_db, sample_s ]) def test_should_set_billable_units_to_zero_in_research_mode_or_test_key( notify_db, sample_service, sample_notification, mocker, research_mode, key_type): - provider_stats = provider_statistics_dao.get_provider_statistics(sample_service).all() - assert len(provider_stats) == 0 mocker.patch('app.mmg_client.send_sms') mocker.patch('app.mmg_client.get_name', return_value="mmg") @@ -292,9 +290,6 @@ def test_send_email_to_provider_should_call_research_mode_task_response_task_if_ sample_service.research_mode = True notify_db.session.add(sample_service) notify_db.session.commit() - assert not get_provider_statistics( - sample_email_template.service, - providers=[ses_provider.identifier]).first() send_to_providers.send_email_to_provider( notification @@ -303,9 +298,6 @@ def test_send_email_to_provider_should_call_research_mode_task_response_task_if_ send_to_providers.send_email_response.apply_async.assert_called_once_with( ('ses', str(reference), 'john@smith.com'), queue="research-mode" ) - assert not get_provider_statistics( - sample_email_template.service, - providers=[ses_provider.identifier]).first() persisted_notification = Notification.query.filter_by(id=notification.id).one() assert persisted_notification.to == 'john@smith.com' @@ -315,6 +307,7 @@ def test_send_email_to_provider_should_call_research_mode_task_response_task_if_ assert persisted_notification.created_at <= datetime.utcnow() assert persisted_notification.sent_by == 'ses' assert persisted_notification.reference == str(reference) + assert persisted_notification.billable_units == 0 def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_created(notify_db, notify_db_session, @@ -415,3 +408,39 @@ def test_should_not_set_billable_units_if_research_mode(notify_db, sample_servic persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id) assert persisted_notification.billable_units == 0 + + +@pytest.mark.parametrize('research_mode,key_type, billable_units', [ + (True, KEY_TYPE_NORMAL, 0), + (False, KEY_TYPE_NORMAL, 1), + (False, KEY_TYPE_TEST, 0), + (True, KEY_TYPE_TEST, 0), + (True, KEY_TYPE_TEAM, 0), + (False, KEY_TYPE_TEAM, 1) +]) +def test_should_update_billable_units_according_to_research_mode_and_key_type(notify_db, + sample_service, + sample_notification, + mocker, + research_mode, + key_type, + billable_units): + + assert Notification.query.count() == 1 + + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.mmg_client.get_name', return_value="mmg") + mocker.patch('app.celery.research_mode_tasks.send_sms_response.apply_async') + if research_mode: + sample_service.research_mode = True + notify_db.session.add(sample_service) + notify_db.session.commit() + + sample_notification.key_type = key_type + + send_to_providers.send_sms_to_provider( + sample_notification + ) + + assert Notification.query.get(sample_notification.id).billable_units == billable_units, \ + "Research mode: {0}, key type: {1}, billable_units: {2}".format(research_mode, key_type, billable_units) diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index eac7aa477..1d8cc85ca 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -3,7 +3,7 @@ import uuid from datetime import datetime from flask import json from freezegun import freeze_time -from mock import call +from unittest.mock import call import app.celery.tasks from app.dao.notifications_dao import ( diff --git a/tests/app/public_contracts/schemas/GET_notifications_return.json b/tests/app/public_contracts/schemas/GET_notifications_return.json index 7939dd8f1..709cca9fe 100644 --- a/tests/app/public_contracts/schemas/GET_notifications_return.json +++ b/tests/app/public_contracts/schemas/GET_notifications_return.json @@ -14,6 +14,17 @@ }, "links": { "type": "object", + "properties" : { + "prev" : { + "type" : "string" + }, + "next" : { + "type" : "string" + }, + "last": { + "type" : "string" + } + }, "additionalProperties": false }, "page_size": {"type": "number"}, diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 6355520e4..ac8c58e3f 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1059,6 +1059,27 @@ def test_get_all_notifications_for_service_including_ones_made_by_jobs( assert response.status_code == 200 +def test_get_only_api_created_notifications_for_service( + client, + notify_db, + notify_db_session, + sample_service +): + with_job = sample_notification_with_job(notify_db, notify_db_session, service=sample_service) + without_job = create_sample_notification(notify_db, notify_db_session, service=sample_service) + + auth_header = create_authorization_header() + + response = client.get( + path='/service/{}/notifications?include_jobs=false'.format(sample_service.id), + headers=[auth_header]) + + resp = json.loads(response.get_data(as_text=True)) + assert len(resp['notifications']) == 1 + assert resp['notifications'][0]['id'] == str(without_job.id) + assert response.status_code == 200 + + def test_set_sms_sender_for_service(notify_api, sample_service): with notify_api.test_request_context(): with notify_api.test_client() as client: