From 6fb4e1606773b4430a9474af5742dc6afed3850b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 12 Jul 2017 14:19:39 +0100 Subject: [PATCH 1/8] Added logging to show the entire form posted to us by the SMS client providers. This can be useful information when debugging what happened to a notificaiton. Recently there was a discrepancy between the failure type used by each provider for a particular number, this logging would have helped. --- app/notifications/notifications_sms_callback.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index 3c569e158..430e1c149 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/app/notifications/notifications_sms_callback.py @@ -1,4 +1,5 @@ from flask import Blueprint +from flask import current_app from flask import json from flask import request, jsonify @@ -22,6 +23,10 @@ def process_mmg_response(): success, errors = process_sms_client_response(status=str(data.get('status')), reference=data.get('CID'), client_name=client_name) + + current_app.logger.info( + "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('reference'), + request.form)) if errors: raise InvalidRequest(errors, status_code=400) else: @@ -38,6 +43,9 @@ def process_firetext_response(): raise InvalidRequest(errors, status_code=400) status = request.form.get('status') + current_app.logger.info( + "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('reference'), + request.form)) success, errors = process_sms_client_response(status=status, reference=request.form.get('reference'), client_name=client_name) From d18ce47114af7fa29967878a5c17a3bc88485c6c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 12 Jul 2017 15:32:59 +0100 Subject: [PATCH 2/8] Make sure we don't log the phone number --- app/notifications/notifications_sms_callback.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index 430e1c149..5120a48c8 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/app/notifications/notifications_sms_callback.py @@ -24,9 +24,11 @@ def process_mmg_response(): reference=data.get('CID'), client_name=client_name) + safe_to_log = data.copy() + safe_to_log.pop("MSISDN") current_app.logger.info( - "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('reference'), - request.form)) + "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('CID'), + safe_to_log)) if errors: raise InvalidRequest(errors, status_code=400) else: @@ -41,12 +43,12 @@ def process_firetext_response(): client_name=client_name) if errors: raise InvalidRequest(errors, status_code=400) - - status = request.form.get('status') + safe_to_log = dict(request.form).copy() + safe_to_log.pop('mobile') current_app.logger.info( "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('reference'), - request.form)) - success, errors = process_sms_client_response(status=status, + safe_to_log)) + success, errors = process_sms_client_response(status=request.form.get('status'), reference=request.form.get('reference'), client_name=client_name) if errors: From 49d1f52aef73e75b754c4db82af66924af9f07d6 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 12 Jul 2017 15:49:43 +0100 Subject: [PATCH 3/8] Fix code style --- app/notifications/notifications_sms_callback.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index 5120a48c8..4cf954840 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/app/notifications/notifications_sms_callback.py @@ -28,7 +28,7 @@ def process_mmg_response(): safe_to_log.pop("MSISDN") current_app.logger.info( "Full delivery response from {} for notification: {}\n{}".format(client_name, request.form.get('CID'), - safe_to_log)) + safe_to_log)) if errors: raise InvalidRequest(errors, status_code=400) else: From 6c61a3fc2ae33324f9530be5379a826d9991c19b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 19 Jul 2017 13:50:29 +0100 Subject: [PATCH 4/8] Revert celery4 Revert the following three pull requests: https://github.com/alphagov/notifications-api/pull/1085 https://github.com/alphagov/notifications-api/pull/1086 https://github.com/alphagov/notifications-api/pull/1088 celery 4.0.2 looked promising, however, on staging under mild load (5/sec api calls) the performance was actually worse than 3.1.25 --- app/__init__.py | 1 + app/celery/__init__.py | 27 ---- app/celery/celery.py | 126 +-------------- app/celery/provider_tasks.py | 2 +- app/celery/research_mode_tasks.py | 1 + app/celery/scheduled_tasks.py | 2 +- app/celery/statistics_tasks.py | 2 +- app/celery/tasks.py | 2 +- app/config.py | 144 +++++++++++++++++- app/delivery/rest.py | 2 +- app/invite/rest.py | 2 +- app/job/rest.py | 2 +- app/letters/send_letter_jobs.py | 2 +- .../notifications_letter_callback.py | 2 +- app/notifications/process_notifications.py | 2 +- app/notifications/receive_notifications.py | 2 +- app/notifications/rest.py | 2 +- app/service/send_notification.py | 2 +- app/service/sender.py | 2 +- app/user/rest.py | 2 +- app/v2/notifications/post_notifications.py | 7 +- docker/Dockerfile | 1 - requirements.txt | 11 +- tests/app/celery/test_statistics_tasks.py | 15 +- .../service/test_send_one_off_notification.py | 2 +- 25 files changed, 173 insertions(+), 192 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 72b8f2885..e0ec02723 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -49,6 +49,7 @@ def create_app(app_name=None): from app.config import configs notify_environment = os.environ['NOTIFY_ENVIRONMENT'] + application.config.from_object(configs[notify_environment]) if app_name: diff --git a/app/celery/__init__.py b/app/celery/__init__.py index 270bb54dd..e69de29bb 100644 --- a/app/celery/__init__.py +++ b/app/celery/__init__.py @@ -1,27 +0,0 @@ - -class QueueNames(object): - PERIODIC = 'periodic-tasks' - PRIORITY = 'priority-tasks' - DATABASE = 'database-tasks' - SEND = 'send-tasks' - RESEARCH_MODE = 'research-mode-tasks' - STATISTICS = 'statistics-tasks' - JOBS = 'job-tasks' - RETRY = 'retry-tasks' - NOTIFY = 'notify-internal-tasks' - PROCESS_FTP = 'process-ftp-tasks' - - @staticmethod - def all_queues(): - return [ - QueueNames.PRIORITY, - QueueNames.PERIODIC, - QueueNames.DATABASE, - QueueNames.SEND, - QueueNames.RESEARCH_MODE, - QueueNames.STATISTICS, - QueueNames.JOBS, - QueueNames.RETRY, - QueueNames.NOTIFY, - QueueNames.PROCESS_FTP - ] diff --git a/app/celery/celery.py b/app/celery/celery.py index d723bfdaa..183e50bd6 100644 --- a/app/celery/celery.py +++ b/app/celery/celery.py @@ -1,130 +1,11 @@ -from datetime import timedelta - from celery import Celery -from celery.schedules import crontab -from kombu import Queue, Exchange - -from app.celery import QueueNames - - -class CeleryConfig: - def __init__(self, config): - self.broker_transport_options['queue_name_prefix'] = config['NOTIFICATION_QUEUE_PREFIX'] - self.broker_url = config.get('BROKER_URL', 'sqs://') - - broker_transport_options = { - 'region': 'eu-west-1', - 'polling_interval': 1, # 1 second - 'visibility_timeout': 310, - 'queue_name_prefix': None - } - enable_utc = True, - timezone = 'Europe/London' - accept_content = ['json'] - task_serializer = 'json' - imports = ('app.celery.tasks', 'app.celery.scheduled_tasks') - beat_schedule = { - 'run-scheduled-jobs': { - 'task': 'run-scheduled-jobs', - 'schedule': crontab(minute=1), - 'options': {'queue': QueueNames.PERIODIC} - }, - # 'send-scheduled-notifications': { - # 'task': 'send-scheduled-notifications', - # 'schedule': crontab(minute='*/15'), - # 'options': {'queue': 'periodic'} - # }, - 'delete-verify-codes': { - 'task': 'delete-verify-codes', - 'schedule': timedelta(minutes=63), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete-invitations': { - 'task': 'delete-invitations', - 'schedule': timedelta(minutes=66), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete-sms-notifications': { - 'task': 'delete-sms-notifications', - 'schedule': crontab(minute=0, hour=0), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete-email-notifications': { - 'task': 'delete-email-notifications', - 'schedule': crontab(minute=20, hour=0), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete-letter-notifications': { - 'task': 'delete-letter-notifications', - 'schedule': crontab(minute=40, hour=0), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete-inbound-sms': { - 'task': 'delete-inbound-sms', - 'schedule': crontab(minute=0, hour=1), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'send-daily-performance-platform-stats': { - 'task': 'send-daily-performance-platform-stats', - 'schedule': crontab(minute=0, hour=2), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'switch-current-sms-provider-on-slow-delivery': { - 'task': 'switch-current-sms-provider-on-slow-delivery', - 'schedule': crontab(), # Every minute - 'options': {'queue': QueueNames.PERIODIC} - }, - 'timeout-sending-notifications': { - 'task': 'timeout-sending-notifications', - 'schedule': crontab(minute=0, hour=3), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'remove_sms_email_jobs': { - 'task': 'remove_csv_files', - 'schedule': crontab(minute=0, hour=4), - 'options': {'queue': QueueNames.PERIODIC}, - # TODO: Avoid duplication of keywords - ideally by moving definitions out of models.py - 'kwargs': {'job_types': ['email', 'sms']} - }, - 'remove_letter_jobs': { - 'task': 'remove_csv_files', - 'schedule': crontab(minute=20, hour=4), - 'options': {'queue': QueueNames.PERIODIC}, - # TODO: Avoid duplication of keywords - ideally by moving definitions out of models.py - 'kwargs': {'job_types': ['letter']} - }, - 'remove_transformed_dvla_files': { - 'task': 'remove_transformed_dvla_files', - 'schedule': crontab(minute=40, hour=4), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'delete_dvla_response_files': { - 'task': 'delete_dvla_response_files', - 'schedule': crontab(minute=10, hour=5), - 'options': {'queue': QueueNames.PERIODIC} - }, - 'timeout-job-statistics': { - 'task': 'timeout-job-statistics', - 'schedule': crontab(minute=0, hour=5), - 'options': {'queue': QueueNames.PERIODIC} - } - } - task_queues = [] class NotifyCelery(Celery): + def init_app(self, app): - celery_config = CeleryConfig(app.config) - - super().__init__(app.import_name, broker=celery_config.broker_url) - - if app.config['INITIALISE_QUEUES']: - for queue in QueueNames.all_queues(): - CeleryConfig.task_queues.append( - Queue(queue, Exchange('default'), routing_key=queue) - ) - - self.config_from_object(celery_config) + super().__init__(app.import_name, broker=app.config['BROKER_URL']) + self.conf.update(app.config) TaskBase = self.Task class ContextTask(TaskBase): @@ -133,5 +14,4 @@ class NotifyCelery(Celery): def __call__(self, *args, **kwargs): with app.app_context(): return TaskBase.__call__(self, *args, **kwargs) - self.Task = ContextTask diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index e748c3bd2..50d5a31b7 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -3,7 +3,7 @@ from notifications_utils.recipients import InvalidEmailError from sqlalchemy.orm.exc import NoResultFound from app import notify_celery -from app.celery import QueueNames +from app.config import QueueNames from app.dao import notifications_dao from app.dao.notifications_dao import update_notification_status_by_id from app.statsd_decorators import statsd diff --git a/app/celery/research_mode_tasks.py b/app/celery/research_mode_tasks.py index 356bc3488..ced35b1d4 100644 --- a/app/celery/research_mode_tasks.py +++ b/app/celery/research_mode_tasks.py @@ -1,6 +1,7 @@ import json from flask import current_app +from app import notify_celery from requests import request, RequestException, HTTPError from app.models import SMS_TYPE diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index b14f7a5be..e86a7c113 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -28,7 +28,7 @@ from app.models import LETTER_TYPE from app.notifications.process_notifications import send_notification_to_queue from app.statsd_decorators import statsd from app.celery.tasks import process_job -from app.celery import QueueNames +from app.config import QueueNames @notify_celery.task(name="remove_csv_files") diff --git a/app/celery/statistics_tasks.py b/app/celery/statistics_tasks.py index b00f88bdd..150fa6aac 100644 --- a/app/celery/statistics_tasks.py +++ b/app/celery/statistics_tasks.py @@ -10,7 +10,7 @@ from app.dao.statistics_dao import ( ) from app.dao.notifications_dao import get_notification_by_id from app.models import NOTIFICATION_STATUS_TYPES_COMPLETED -from app.celery import QueueNames +from app.config import QueueNames def create_initial_notification_statistic_tasks(notification): diff --git a/app/celery/tasks.py b/app/celery/tasks.py index bbd1fa207..da9d1f9ee 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -19,8 +19,8 @@ from app import ( ) from app.aws import s3 from app.celery import provider_tasks +from app.config import QueueNames from app.dao.inbound_sms_dao import dao_get_inbound_sms_by_id -from app.celery import QueueNames from app.dao.jobs_dao import ( dao_update_job, dao_get_job_by_id, diff --git a/app/config.py b/app/config.py index 290cc3211..1bbbb2057 100644 --- a/app/config.py +++ b/app/config.py @@ -1,6 +1,10 @@ +from datetime import timedelta import os import json +from celery.schedules import crontab +from kombu import Exchange, Queue + from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST @@ -14,6 +18,34 @@ if os.environ.get('VCAP_SERVICES'): extract_cloudfoundry_config() +class QueueNames(object): + PERIODIC = 'periodic-tasks' + PRIORITY = 'priority-tasks' + DATABASE = 'database-tasks' + SEND = 'send-tasks' + RESEARCH_MODE = 'research-mode-tasks' + STATISTICS = 'statistics-tasks' + JOBS = 'job-tasks' + RETRY = 'retry-tasks' + NOTIFY = 'notify-internal-tasks' + PROCESS_FTP = 'process-ftp-tasks' + + @staticmethod + def all_queues(): + return [ + QueueNames.PRIORITY, + QueueNames.PERIODIC, + QueueNames.DATABASE, + QueueNames.SEND, + QueueNames.RESEARCH_MODE, + QueueNames.STATISTICS, + QueueNames.JOBS, + QueueNames.RETRY, + QueueNames.NOTIFY, + QueueNames.PROCESS_FTP + ] + + class Config(object): # URL of admin app ADMIN_BASE_URL = os.environ['ADMIN_BASE_URL'] @@ -94,6 +126,104 @@ class Config(object): CHANGE_EMAIL_CONFIRMATION_TEMPLATE_ID = 'eb4d9930-87ab-4aef-9bce-786762687884' SERVICE_NOW_LIVE_TEMPLATE_ID = '618185c6-3636-49cd-b7d2-6f6f5eb3bdde' + BROKER_URL = 'sqs://' + BROKER_TRANSPORT_OPTIONS = { + 'region': AWS_REGION, + 'polling_interval': 1, # 1 second + 'visibility_timeout': 310, + 'queue_name_prefix': NOTIFICATION_QUEUE_PREFIX + } + CELERY_ENABLE_UTC = True, + CELERY_TIMEZONE = 'Europe/London' + CELERY_ACCEPT_CONTENT = ['json'] + CELERY_TASK_SERIALIZER = 'json' + CELERY_IMPORTS = ('app.celery.tasks', 'app.celery.scheduled_tasks') + CELERYBEAT_SCHEDULE = { + 'run-scheduled-jobs': { + 'task': 'run-scheduled-jobs', + 'schedule': crontab(minute=1), + 'options': {'queue': QueueNames.PERIODIC} + }, + # 'send-scheduled-notifications': { + # 'task': 'send-scheduled-notifications', + # 'schedule': crontab(minute='*/15'), + # 'options': {'queue': 'periodic'} + # }, + 'delete-verify-codes': { + 'task': 'delete-verify-codes', + 'schedule': timedelta(minutes=63), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-invitations': { + 'task': 'delete-invitations', + 'schedule': timedelta(minutes=66), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-sms-notifications': { + 'task': 'delete-sms-notifications', + 'schedule': crontab(minute=0, hour=0), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-email-notifications': { + 'task': 'delete-email-notifications', + 'schedule': crontab(minute=20, hour=0), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-letter-notifications': { + 'task': 'delete-letter-notifications', + 'schedule': crontab(minute=40, hour=0), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete-inbound-sms': { + 'task': 'delete-inbound-sms', + 'schedule': crontab(minute=0, hour=1), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'send-daily-performance-platform-stats': { + 'task': 'send-daily-performance-platform-stats', + 'schedule': crontab(minute=0, hour=2), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'switch-current-sms-provider-on-slow-delivery': { + 'task': 'switch-current-sms-provider-on-slow-delivery', + 'schedule': crontab(), # Every minute + 'options': {'queue': QueueNames.PERIODIC} + }, + 'timeout-sending-notifications': { + 'task': 'timeout-sending-notifications', + 'schedule': crontab(minute=0, hour=3), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'remove_sms_email_jobs': { + 'task': 'remove_csv_files', + 'schedule': crontab(minute=0, hour=4), + 'options': {'queue': QueueNames.PERIODIC}, + 'kwargs': {'job_types': [EMAIL_TYPE, SMS_TYPE]} + }, + 'remove_letter_jobs': { + 'task': 'remove_csv_files', + 'schedule': crontab(minute=20, hour=4), + 'options': {'queue': QueueNames.PERIODIC}, + 'kwargs': {'job_types': [LETTER_TYPE]} + }, + 'remove_transformed_dvla_files': { + 'task': 'remove_transformed_dvla_files', + 'schedule': crontab(minute=40, hour=4), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'delete_dvla_response_files': { + 'task': 'delete_dvla_response_files', + 'schedule': crontab(minute=10, hour=5), + 'options': {'queue': QueueNames.PERIODIC} + }, + 'timeout-job-statistics': { + 'task': 'timeout-job-statistics', + 'schedule': crontab(minute=0, hour=5), + 'options': {'queue': QueueNames.PERIODIC} + } + } + CELERY_QUEUES = [] + NOTIFICATIONS_ALERT = 5 # five mins FROM_NUMBER = 'development' @@ -132,7 +262,6 @@ class Config(object): } FREE_SMS_TIER_FRAGMENT_COUNT = 250000 - INITIALISE_QUEUES = False SMS_INBOUND_WHITELIST = json.loads(os.environ.get('SMS_INBOUND_WHITELIST', '[]')) @@ -142,20 +271,24 @@ class Config(object): ###################### class Development(Config): - INITIALISE_QUEUES = True SQLALCHEMY_ECHO = False NOTIFY_EMAIL_DOMAIN = 'notify.tools' CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' DVLA_RESPONSE_BUCKET_NAME = 'notify.tools-ftp' NOTIFY_ENVIRONMENT = 'development' + NOTIFICATION_QUEUE_PREFIX = 'development' DEBUG = True + for queue in QueueNames.all_queues(): + Config.CELERY_QUEUES.append( + Queue(queue, Exchange('default'), routing_key=queue) + ) + API_HOST_NAME = "http://localhost:6011" API_RATE_LIMIT_ENABLED = True class Test(Config): - INITIALISE_QUEUES = True NOTIFY_EMAIL_DOMAIN = 'test.notify.com' FROM_NUMBER = 'testing' NOTIFY_ENVIRONMENT = 'test' @@ -169,6 +302,11 @@ class Test(Config): BROKER_URL = 'you-forgot-to-mock-celery-in-your-tests://' + for queue in QueueNames.all_queues(): + Config.CELERY_QUEUES.append( + Queue(queue, Exchange('default'), routing_key=queue) + ) + API_RATE_LIMIT_ENABLED = True API_HOST_NAME = "http://localhost:6011" diff --git a/app/delivery/rest.py b/app/delivery/rest.py index 5f4abb70b..489a5fcda 100644 --- a/app/delivery/rest.py +++ b/app/delivery/rest.py @@ -1,6 +1,6 @@ from flask import Blueprint, jsonify -from app.celery import QueueNames +from app.config import QueueNames from app.delivery import send_to_providers from app.models import EMAIL_TYPE from app.celery import provider_tasks diff --git a/app/invite/rest.py b/app/invite/rest.py index e1e401190..8105b171f 100644 --- a/app/invite/rest.py +++ b/app/invite/rest.py @@ -4,7 +4,7 @@ from flask import ( jsonify, current_app) -from app.celery import QueueNames +from app.config import QueueNames from app.dao.invited_user_dao import ( save_invited_user, get_invited_user, diff --git a/app/job/rest.py b/app/job/rest.py index e25aa5fcb..6a8a86ee4 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -36,7 +36,7 @@ from app.models import JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING, JOB_STATUS_CANC from app.utils import pagination_links -from app.celery import QueueNames +from app.config import QueueNames job_blueprint = Blueprint('job', __name__, url_prefix='/service//job') diff --git a/app/letters/send_letter_jobs.py b/app/letters/send_letter_jobs.py index f90d7f280..91c39615a 100644 --- a/app/letters/send_letter_jobs.py +++ b/app/letters/send_letter_jobs.py @@ -2,7 +2,7 @@ from flask import Blueprint, jsonify from flask import request from app import notify_celery -from app.celery import QueueNames +from app.config import QueueNames from app.dao.jobs_dao import dao_get_all_letter_jobs from app.schemas import job_schema from app.v2.errors import register_errors diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index b8b587cf0..ac2de9e5c 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/app/notifications/notifications_letter_callback.py @@ -13,7 +13,7 @@ from app.celery.tasks import update_letter_notifications_statuses from app.v2.errors import register_errors from app.notifications.utils import autoconfirm_subscription from app.schema_validation import validate -from app.celery import QueueNames +from app.config import QueueNames letter_callback_blueprint = Blueprint('notifications_letter_callback', __name__) register_errors(letter_callback_blueprint) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index da757db4b..ca3ae0b53 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -12,7 +12,7 @@ from app import redis_store from app.celery import provider_tasks from notifications_utils.clients import redis -from app.celery import QueueNames +from app.config import QueueNames from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE, ScheduledNotification from app.dao.notifications_dao import (dao_create_notification, dao_delete_notifications_and_history_by_id, diff --git a/app/notifications/receive_notifications.py b/app/notifications/receive_notifications.py index 07847a45c..34e726cf1 100644 --- a/app/notifications/receive_notifications.py +++ b/app/notifications/receive_notifications.py @@ -6,7 +6,7 @@ from notifications_utils.recipients import validate_and_format_phone_number from app import statsd_client, firetext_client, mmg_client from app.celery import tasks -from app.celery import QueueNames +from app.config import QueueNames from app.dao.services_dao import dao_fetch_services_by_sms_sender from app.dao.inbound_sms_dao import dao_create_inbound_sms from app.models import InboundSms, INBOUND_SMS_TYPE, SMS_TYPE diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 16ba13fee..5c7f8fdea 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -6,7 +6,7 @@ from flask import ( ) from app import api_user, authenticated_service -from app.celery import QueueNames +from app.config import QueueNames from app.dao import ( templates_dao, notifications_dao diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 6119ac714..1ca19661c 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -1,4 +1,4 @@ -from app.celery import QueueNames +from app.config import QueueNames from app.notifications.validators import ( check_service_over_daily_message_limit, validate_and_format_recipient, diff --git a/app/service/sender.py b/app/service/sender.py index 3c4ef17d9..4919a93bf 100644 --- a/app/service/sender.py +++ b/app/service/sender.py @@ -1,6 +1,6 @@ from flask import current_app -from app.celery import QueueNames +from app.config import QueueNames from app.dao.services_dao import dao_fetch_service_by_id, dao_fetch_active_users_for_service from app.dao.templates_dao import dao_get_template_by_id from app.models import EMAIL_TYPE, KEY_TYPE_NORMAL diff --git a/app/user/rest.py b/app/user/rest.py index 45c649940..9a9cf3832 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -4,7 +4,7 @@ from datetime import datetime from flask import (jsonify, request, Blueprint, current_app) -from app.celery import QueueNames +from app.config import QueueNames from app.dao.users_dao import ( get_user_by_id, save_model_user, diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index f9da36f88..274005386 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -1,8 +1,8 @@ from flask import request, jsonify, current_app from app import api_user, authenticated_service -from app.models import SMS_TYPE, EMAIL_TYPE, PRIORITY -from app.celery import QueueNames +from app.config import QueueNames +from app.models import SMS_TYPE, EMAIL_TYPE, PRIORITY, SCHEDULE_NOTIFICATIONS from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue, @@ -11,17 +11,20 @@ from app.notifications.process_notifications import ( from app.notifications.validators import ( validate_and_format_recipient, check_rate_limiting, + service_has_permission, check_service_can_schedule_notification, check_service_has_permission, validate_template ) from app.schema_validation import validate +from app.utils import get_public_notify_type_text from app.v2.notifications import v2_notification_blueprint from app.v2.notifications.notification_schemas import ( post_sms_request, create_post_sms_response_from_notification, post_email_request, create_post_email_response_from_notification) +from app.v2.errors import BadRequestError @v2_notification_blueprint.route('/', methods=['POST']) diff --git a/docker/Dockerfile b/docker/Dockerfile index 0a96a16f6..2ccfe0e1e 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -20,7 +20,6 @@ RUN \ zip \ libpq-dev \ jq \ - libcurl4-openssl-dev \ && echo "Clean up" \ && rm -rf /var/lib/apt/lists/* /tmp/* diff --git a/requirements.txt b/requirements.txt index de027e13c..b1d1cd307 100644 --- a/requirements.txt +++ b/requirements.txt @@ -13,7 +13,7 @@ flask-marshmallow==0.6.2 Flask-Bcrypt==0.6.2 credstash==1.8.0 boto3==1.4.4 -celery==4.0.2 +celery==3.1.25 monotonic==1.2 statsd==3.2.1 jsonschema==2.5.1 @@ -21,7 +21,6 @@ gunicorn==19.6.0 docopt==0.6.2 six==1.10.0 iso8601==0.1.11 -pycurl==7.43.0 # pin to minor version 3.1.x notifications-python-client>=3.1,<3.2 @@ -32,10 +31,4 @@ awscli-cwlogs>=1.4,<1.5 git+https://github.com/alphagov/notifications-utils.git@17.5.3#egg=notifications-utils==17.5.3 -# Kombu is a library that celery uses under the hood. -# Kombu v4.0.2 (which ships with celery v4.0.2) doesn't work with SQS due to problems with their use of boto2, so -# Kombu migrated to boto3 - We're waiting for that to get a release version, and then to get a new version of celery -# that pulls that in. Until that point, we should override the kombu version to get these SQS fixes. -# Additionally, kombu master also includes a fix for the main process taking 100% CPU and not distributing tasks (!) -# See https://github.com/celery/kombu/pull/693 and https://github.com/celery/kombu/pull/760 -https://github.com/celery/kombu/zipball/b2f21289284496efd89acea003ff9c24105b970e +git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/celery/test_statistics_tasks.py b/tests/app/celery/test_statistics_tasks.py index c97007f55..40d20117d 100644 --- a/tests/app/celery/test_statistics_tasks.py +++ b/tests/app/celery/test_statistics_tasks.py @@ -1,21 +1,14 @@ import pytest -from sqlalchemy.exc import SQLAlchemyError - -from app import create_uuid from app.celery.statistics_tasks import ( record_initial_job_statistics, record_outcome_job_statistics, create_initial_notification_statistic_tasks, create_outcome_notification_statistic_tasks) -from app.models import ( - NOTIFICATION_STATUS_TYPES_COMPLETED, - NOTIFICATION_SENDING, - NOTIFICATION_PENDING, - NOTIFICATION_CREATED, - NOTIFICATION_DELIVERED -) - +from sqlalchemy.exc import SQLAlchemyError +from app import create_uuid from tests.app.conftest import sample_notification +from app.models import NOTIFICATION_STATUS_TYPES_COMPLETED, NOTIFICATION_SENT, NOTIFICATION_SENDING, \ + NOTIFICATION_PENDING, NOTIFICATION_CREATED, NOTIFICATION_DELIVERED def test_should_create_initial_job_task_if_notification_is_related_to_a_job( diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index 3baf886e7..89ddcd91a 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -5,7 +5,7 @@ import pytest from notifications_utils.recipients import InvalidPhoneError from app.v2.errors import BadRequestError, TooManyRequestsError -from app.celery import QueueNames +from app.config import QueueNames from app.service.send_notification import send_one_off_notification from app.models import KEY_TYPE_NORMAL, PRIORITY, SMS_TYPE From 4d330406534ba10b0fcdc757daa4237adf16cfe1 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 20 Jul 2017 15:48:21 +0100 Subject: [PATCH 5/8] add separate send-sms and send-email queues we're reading from those two queues as well as teh existing send queue, however for now we don't send anything to them --- app/celery/tasks.py | 4 ++-- app/config.py | 8 ++++++-- app/delivery/rest.py | 2 +- app/notifications/process_notifications.py | 2 +- manifest-delivery-base.yml | 2 +- 5 files changed, 11 insertions(+), 7 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index da9d1f9ee..c3f533a6d 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -182,7 +182,7 @@ def send_sms(self, provider_tasks.deliver_sms.apply_async( [str(saved_notification.id)], - queue=QueueNames.SEND if not service.research_mode else QueueNames.RESEARCH_MODE + queue=QueueNames.SEND_COMBINED if not service.research_mode else QueueNames.RESEARCH_MODE ) current_app.logger.info( @@ -227,7 +227,7 @@ def send_email(self, provider_tasks.deliver_email.apply_async( [str(saved_notification.id)], - queue=QueueNames.SEND if not service.research_mode else QueueNames.RESEARCH_MODE + queue=QueueNames.SEND_COMBINED if not service.research_mode else QueueNames.RESEARCH_MODE ) current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at)) diff --git a/app/config.py b/app/config.py index 1bbbb2057..99b77cc8d 100644 --- a/app/config.py +++ b/app/config.py @@ -22,7 +22,9 @@ class QueueNames(object): PERIODIC = 'periodic-tasks' PRIORITY = 'priority-tasks' DATABASE = 'database-tasks' - SEND = 'send-tasks' + SEND_COMBINED = 'send-tasks' + SEND_SMS = 'send-sms-tasks' + SEND_EMAIL = 'send-email-tasks' RESEARCH_MODE = 'research-mode-tasks' STATISTICS = 'statistics-tasks' JOBS = 'job-tasks' @@ -36,7 +38,9 @@ class QueueNames(object): QueueNames.PRIORITY, QueueNames.PERIODIC, QueueNames.DATABASE, - QueueNames.SEND, + QueueNames.SEND_COMBINED, + QueueNames.SEND_SMS, + QueueNames.SEND_EMAIL, QueueNames.RESEARCH_MODE, QueueNames.STATISTICS, QueueNames.JOBS, diff --git a/app/delivery/rest.py b/app/delivery/rest.py index 489a5fcda..4dba91208 100644 --- a/app/delivery/rest.py +++ b/app/delivery/rest.py @@ -42,4 +42,4 @@ def send_response(send_call, task_call, notification): notification.id, notification.notification_type), e) - task_call.apply_async((str(notification.id)), queue=QueueNames.SEND) + task_call.apply_async((str(notification.id)), queue=QueueNames.SEND_COMBINED) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index ca3ae0b53..78881d2b6 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -101,7 +101,7 @@ def send_notification_to_queue(notification, research_mode, queue=None): if research_mode or notification.key_type == KEY_TYPE_TEST: queue = QueueNames.RESEARCH_MODE elif not queue: - queue = QueueNames.SEND + queue = QueueNames.SEND_COMBINED if notification.notification_type == SMS_TYPE: deliver_task = provider_tasks.deliver_sms diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 7f73a4b54..597df9f3f 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -33,7 +33,7 @@ applications: NOTIFY_APP_NAME: delivery-worker-research - name: notify-delivery-worker-sender - command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q send-tasks + command: scripts/run_app_paas.sh celery -A aws_run_celery.notify_celery worker --loglevel=INFO --concurrency=11 -Q send-tasks,send-sms-tasks,send-email-tasks env: NOTIFY_APP_NAME: delivery-worker-sender From e65619cb9020bed01ee00b49da22795c9ab8f00f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 17 Jul 2017 09:40:05 +0100 Subject: [PATCH 6/8] Bump utils Brings in: - [ ] https://github.com/alphagov/notifications-utils/pull/182 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index b1d1cd307..875e27a94 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@17.5.3#egg=notifications-utils==17.5.3 +git+https://github.com/alphagov/notifications-utils.git@17.5.7#egg=notifications-utils==17.5.7 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From d4bbca259232a0d1ab14532802778073cc026aa7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 20 Jul 2017 17:56:51 +0100 Subject: [PATCH 7/8] Fix for simulated notifications. When a post is made for a simulated number the id is empty in the notificaiton object that we return. This fixes that. --- app/notifications/process_notifications.py | 4 +++- tests/app/v2/notifications/test_post_notifications.py | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index ca3ae0b53..950df8304 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -1,3 +1,4 @@ +import uuid from datetime import datetime from flask import current_app @@ -53,7 +54,8 @@ def persist_notification( created_by_id=None ): notification_created_at = created_at or datetime.utcnow() - + if not notification_id and simulated: + notification_id = uuid.uuid4() notification = Notification( id=notification_id, template_id=template_id, diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index aea003b7e..7433b9bcf 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -201,6 +201,7 @@ def test_should_not_persist_or_send_notification_if_simulated_recipient( assert response.status_code == 201 apply_async.assert_not_called() + assert json.loads(response.get_data(as_text=True))["id"] assert Notification.query.count() == 0 From 614880f6d9f0da758bd9e52dc484da8b77212124 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 20 Jul 2017 16:17:04 +0100 Subject: [PATCH 8/8] send to send-sms-tasks and send-email-tasks instead of send-tasks --- app/celery/tasks.py | 4 +- app/delivery/rest.py | 12 +- app/notifications/process_notifications.py | 6 +- tests/app/celery/test_scheduled_tasks.py | 2 +- tests/app/celery/test_tasks.py | 18 +-- tests/app/delivery/test_rest.py | 4 +- .../rest/test_send_notification.py | 109 ++++++++++-------- .../test_process_notification.py | 6 +- 8 files changed, 90 insertions(+), 71 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index c3f533a6d..008b0fd5e 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -182,7 +182,7 @@ def send_sms(self, provider_tasks.deliver_sms.apply_async( [str(saved_notification.id)], - queue=QueueNames.SEND_COMBINED if not service.research_mode else QueueNames.RESEARCH_MODE + queue=QueueNames.SEND_SMS if not service.research_mode else QueueNames.RESEARCH_MODE ) current_app.logger.info( @@ -227,7 +227,7 @@ def send_email(self, provider_tasks.deliver_email.apply_async( [str(saved_notification.id)], - queue=QueueNames.SEND_COMBINED if not service.research_mode else QueueNames.RESEARCH_MODE + queue=QueueNames.SEND_EMAIL if not service.research_mode else QueueNames.RESEARCH_MODE ) current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at)) diff --git a/app/delivery/rest.py b/app/delivery/rest.py index 4dba91208..83236d521 100644 --- a/app/delivery/rest.py +++ b/app/delivery/rest.py @@ -24,16 +24,20 @@ def send_notification_to_provider(notification_id): send_response( send_to_providers.send_email_to_provider, provider_tasks.deliver_email, - notification) + notification, + QueueNames.SEND_EMAIL + ) else: send_response( send_to_providers.send_sms_to_provider, provider_tasks.deliver_sms, - notification) + notification, + QueueNames.SEND_SMS + ) return jsonify({}), 204 -def send_response(send_call, task_call, notification): +def send_response(send_call, task_call, notification, queue): try: send_call(notification) except Exception as e: @@ -42,4 +46,4 @@ def send_response(send_call, task_call, notification): notification.id, notification.notification_type), e) - task_call.apply_async((str(notification.id)), queue=QueueNames.SEND_COMBINED) + task_call.apply_async((str(notification.id)), queue=queue) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 78881d2b6..76da026c4 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -100,12 +100,14 @@ def persist_notification( def send_notification_to_queue(notification, research_mode, queue=None): if research_mode or notification.key_type == KEY_TYPE_TEST: queue = QueueNames.RESEARCH_MODE - elif not queue: - queue = QueueNames.SEND_COMBINED if notification.notification_type == SMS_TYPE: + if not queue: + queue = QueueNames.SEND_SMS deliver_task = provider_tasks.deliver_sms if notification.notification_type == EMAIL_TYPE: + if not queue: + queue = QueueNames.SEND_EMAIL deliver_task = provider_tasks.deliver_email try: diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index b4ad7e255..0282f8074 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -468,7 +468,7 @@ def test_should_send_all_scheduled_notifications_to_deliver_queue(sample_templat send_scheduled_notifications() - mocked.apply_async.assert_called_once_with([str(message_to_deliver.id)], queue='send-tasks') + mocked.apply_async.assert_called_once_with([str(message_to_deliver.id)], queue='send-sms-tasks') scheduled_notifications = dao_get_scheduled_notifications() assert not scheduled_notifications diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 55b154aa9..c6b09e333 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -422,7 +422,7 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi assert persisted_notification.notification_type == 'sms' mocked_deliver_sms.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-sms-tasks" ) @@ -483,7 +483,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif assert persisted_notification.notification_type == 'sms' provider_tasks.deliver_sms.apply_async.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-sms-tasks" ) @@ -509,7 +509,7 @@ def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key persisted_notification = Notification.query.one() mocked_deliver_sms.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-sms-tasks" ) @@ -537,7 +537,7 @@ def test_should_send_email_if_restricted_service_and_non_team_email_address_with persisted_notification = Notification.query.one() mocked_deliver_email.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-email-tasks" ) @@ -641,7 +641,7 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ provider_tasks.deliver_sms.apply_async.assert_called_once_with( [str(persisted_notification.id)], - queue="send-tasks" + queue="send-sms-tasks" ) @@ -738,7 +738,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with( - [str(persisted_notification.id)], queue='send-tasks') + [str(persisted_notification.id)], queue='send-email-tasks') def test_send_email_should_use_template_version_from_job_not_latest(sample_email_template, mocker): @@ -769,7 +769,7 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email assert not persisted_notification.sent_by assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with([str(persisted_notification.id)], - queue='send-tasks') + queue='send-email-tasks') def test_should_use_email_template_subject_placeholders(sample_email_template_with_placeholders, mocker): @@ -795,7 +795,7 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi assert not persisted_notification.reference assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with( - [str(persisted_notification.id)], queue='send-tasks' + [str(persisted_notification.id)], queue='send-email-tasks' ) @@ -823,7 +823,7 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em assert not persisted_notification.reference assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with([str(persisted_notification.id)], - queue='send-tasks') + queue='send-email-tasks') def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, mocker): diff --git a/tests/app/delivery/test_rest.py b/tests/app/delivery/test_rest.py index 984bf90e1..08a5d0a05 100644 --- a/tests/app/delivery/test_rest.py +++ b/tests/app/delivery/test_rest.py @@ -78,7 +78,7 @@ def test_should_call_deliver_sms_task_if_send_sms_to_provider_fails(notify_api, ) app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification) app.celery.provider_tasks.deliver_sms.apply_async.assert_called_with( - (str(sample_notification.id)), queue='send-tasks' + (str(sample_notification.id)), queue='send-sms-tasks' ) assert response.status_code == 204 @@ -100,6 +100,6 @@ def test_should_call_deliver_email_task_if_send_email_to_provider_fails( ) app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_email_notification) app.celery.provider_tasks.deliver_email.apply_async.assert_called_with( - (str(sample_email_notification.id)), queue='send-tasks' + (str(sample_email_notification.id)), queue='send-email-tasks' ) assert response.status_code == 204 diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index aa7ab2d86..3822395cd 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -132,7 +132,7 @@ def test_send_notification_with_placeholders_replaced(notify_api, sample_email_t mocked.assert_called_once_with( [notification_id], - queue="send-tasks" + queue="send-email-tasks" ) assert response.status_code == 201 assert response_data['body'] == u'Hello Jo\nThis is an email from GOV.\u200BUK' @@ -342,7 +342,7 @@ def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker response_data = json.loads(response.data)['data'] notification_id = response_data['notification']['id'] - mocked.assert_called_once_with([notification_id], queue='send-tasks') + mocked.assert_called_once_with([notification_id], queue='send-sms-tasks') assert response.status_code == 201 assert notification_id assert 'subject' not in response_data @@ -395,7 +395,7 @@ def test_should_allow_valid_email_notification(notify_api, sample_email_template notification_id = response_data['notification']['id'] app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with( [notification_id], - queue="send-tasks" + queue="send-email-tasks" ) assert response.status_code == 201 @@ -593,7 +593,10 @@ def test_should_send_email_if_team_api_key_and_a_service_user(notify_api, sample data=json.dumps(data), headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with([fake_uuid], queue='send-tasks') + app.celery.provider_tasks.deliver_email.apply_async.assert_called_once_with( + [fake_uuid], + queue='send-email-tasks' + ) assert response.status_code == 201 @@ -689,57 +692,67 @@ def test_should_send_sms_if_team_api_key_and_a_service_user(notify_api, sample_t data=json.dumps(data), headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with([fake_uuid], queue='send-tasks') + app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with([fake_uuid], queue='send-sms-tasks') assert response.status_code == 201 -@pytest.mark.parametrize('template_type', - [SMS_TYPE, EMAIL_TYPE]) -def test_should_persist_notification(notify_api, sample_template, - sample_email_template, - template_type, - fake_uuid, mocker): - with notify_api.test_request_context(), notify_api.test_client() as client: - mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type)) - mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) - template = sample_template if template_type == SMS_TYPE else sample_email_template - to = sample_template.service.created_by.mobile_number if template_type == SMS_TYPE \ - else sample_email_template.service.created_by.email_address - data = { - 'to': to, - 'template': template.id - } - api_key = ApiKey( - service=template.service, - name='team_key', - created_by=template.created_by, - key_type=KEY_TYPE_TEAM) - save_model_api_key(api_key) - auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) +@pytest.mark.parametrize('template_type,queue_name', [ + (SMS_TYPE, 'send-sms-tasks'), + (EMAIL_TYPE, 'send-email-tasks') +]) +def test_should_persist_notification( + client, + sample_template, + sample_email_template, + fake_uuid, + mocker, + template_type, + queue_name +): + mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type)) + mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) + template = sample_template if template_type == SMS_TYPE else sample_email_template + to = sample_template.service.created_by.mobile_number if template_type == SMS_TYPE \ + else sample_email_template.service.created_by.email_address + data = { + 'to': to, + 'template': template.id + } + api_key = ApiKey( + service=template.service, + name='team_key', + created_by=template.created_by, + key_type=KEY_TYPE_TEAM) + save_model_api_key(api_key) + auth_header = create_jwt_token(secret=api_key.secret, client_id=str(api_key.service_id)) - response = client.post( - path='/notifications/{}'.format(template_type), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + response = client.post( + path='/notifications/{}'.format(template_type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - mocked.assert_called_once_with([fake_uuid], queue='send-tasks') - assert response.status_code == 201 + mocked.assert_called_once_with([fake_uuid], queue=queue_name) + assert response.status_code == 201 - notification = notifications_dao.get_notification_by_id(fake_uuid) - assert notification.to == to - assert notification.template_id == template.id - assert notification.notification_type == template_type + notification = notifications_dao.get_notification_by_id(fake_uuid) + assert notification.to == to + assert notification.template_id == template.id + assert notification.notification_type == template_type -@pytest.mark.parametrize('template_type', - [SMS_TYPE, EMAIL_TYPE]) +@pytest.mark.parametrize('template_type,queue_name', [ + (SMS_TYPE, 'send-sms-tasks'), + (EMAIL_TYPE, 'send-email-tasks') +]) def test_should_delete_notification_and_return_error_if_sqs_fails( - client, - sample_email_template, - sample_template, - fake_uuid, - mocker, - template_type): + client, + sample_email_template, + sample_template, + fake_uuid, + mocker, + template_type, + queue_name +): mocked = mocker.patch( 'app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type), side_effect=Exception("failed to talk to SQS") @@ -768,7 +781,7 @@ def test_should_delete_notification_and_return_error_if_sqs_fails( ) assert str(e.value) == 'failed to talk to SQS' - mocked.assert_called_once_with([fake_uuid], queue='send-tasks') + mocked.assert_called_once_with([fake_uuid], queue=queue_name) assert not notifications_dao.get_notification_by_id(fake_uuid) assert not NotificationHistory.query.get(fake_uuid) @@ -1119,7 +1132,7 @@ def test_should_allow_store_original_number_on_sms_notification(client, sample_t response_data = json.loads(response.data)['data'] notification_id = response_data['notification']['id'] - mocked.assert_called_once_with([notification_id], queue='send-tasks') + mocked.assert_called_once_with([notification_id], queue='send-sms-tasks') assert response.status_code == 201 assert notification_id notifications = Notification.query.all() diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index ac73b36da..3e4d1d9c2 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -214,9 +214,9 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa [(True, None, 'research-mode-tasks', 'sms', 'normal'), (True, None, 'research-mode-tasks', 'email', 'normal'), (True, None, 'research-mode-tasks', 'email', 'team'), - (False, None, 'send-tasks', 'sms', 'normal'), - (False, None, 'send-tasks', 'email', 'normal'), - (False, None, 'send-tasks', 'sms', 'team'), + (False, None, 'send-sms-tasks', 'sms', 'normal'), + (False, None, 'send-email-tasks', 'email', 'normal'), + (False, None, 'send-sms-tasks', 'sms', 'team'), (False, None, 'research-mode-tasks', 'sms', 'test'), (True, 'notify-internal-tasks', 'research-mode-tasks', 'email', 'normal'), (False, 'notify-internal-tasks', 'notify-internal-tasks', 'sms', 'normal'),