Merge branch 'master' into refactor-send-tasks-into-shared-code

Conflicts:
	app/celery/provider_tasks.py
	tests/app/celery/test_provider_tasks.py

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

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,

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'])

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(

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

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

deploy-exclude.lst

@@ -7,3 +7,6 @@
 *node_modules*
 *target*
 *venv*
+*build*
+*.envrc*
+*tests/.cache*

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
+echo "Chown application to be owned by notify-app"
+
+cd /home/notify-app/;
+chown -R notify-app:govuk-notify-applications notifications-api

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
+cd /home/notify-app/notifications-api;
+pip3 install -r /home/notify-app/notifications-api/requirements.txt

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

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"

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"

setup.cfg

@@ -1,4 +1,4 @@
 [pep8]
 max-line-length = 120
 ignore = E402
-exclude = ./migrations,./venv,./venv3
+exclude = ./migrations,./venv,./venv3,./build

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'

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)

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 (

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"},

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: