From fe7d894420a5cf3b1f2eec572b11a4e461e9917b Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Mon, 6 Jun 2016 09:49:51 +0100 Subject: [PATCH 01/13] Replaced mmg from number and firetext from number with single from number. Fix merge mistake. Fix tests from merge. Update config to include correct staging and live names. --- README.md | 3 +-- app/clients/sms/firetext.py | 2 +- app/clients/sms/mmg.py | 2 +- config.py | 4 +--- config_live.py | 2 +- config_staging.py | 2 +- environment_test.sh | 4 +--- tests/app/conftest.py | 4 ++-- 8 files changed, 9 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 8f1f8cdf5..850ffa577 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,6 @@ export ADMIN_CLIENT_USER_NAME='dev-notify-admin' export AWS_REGION='eu-west-1' export DANGEROUS_SALT='dev-notify-salt' export FIRETEXT_API_KEY=[contact team member for api key] -export FIRETEXT_NUMBER="Firetext" export INVITATION_EMAIL_FROM='invites@notifications.service.gov.uk' export INVITATION_EXPIRATION_DAYS=2 export NOTIFY_EMAIL_DOMAIN='notify.works' @@ -41,11 +40,11 @@ export TWILIO_ACCOUNT_SID=[contact team member for account sid] export TWILIO_AUTH_TOKEN=[contact team member for auth token] export VERIFY_CODE_FROM_EMAIL_ADDRESS='no-reply@notify.works' export MMG_API_KEY=mmg=secret-key -export MMG_FROM_NUMBER="MMG export STATSD_ENABLED=True export STATSD_HOST="localhost" export STATSD_PORT=1000 export STATSD_PREFIX="stats-prefix" +export FROM_NUMBER='from_number' "> environment.sh ``` diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 2dfba89f0..16bbcf047 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -60,7 +60,7 @@ class FiretextClient(SmsClient): super(SmsClient, self).__init__(*args, **kwargs) self.current_app = current_app self.api_key = current_app.config.get('FIRETEXT_API_KEY') - self.from_number = current_app.config.get('FIRETEXT_NUMBER') + self.from_number = current_app.config.get('FROM_NUMBER') self.name = 'firetext' self.statsd_client = statsd_client diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 5d4698ba8..8118ec4fd 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -60,7 +60,7 @@ class MMGClient(SmsClient): super(SmsClient, self).__init__(*args, **kwargs) self.current_app = current_app self.api_key = current_app.config.get('MMG_API_KEY') - self.from_number = current_app.config.get('MMG_FROM_NUMBER') + self.from_number = current_app.config.get('FROM_NUMBER') self.name = 'mmg' self.statsd_client = statsd_client diff --git a/config.py b/config.py index 69b0e6694..c83213e41 100644 --- a/config.py +++ b/config.py @@ -18,7 +18,6 @@ class Config(object): NOTIFY_JOB_QUEUE = os.environ['NOTIFY_JOB_QUEUE'] # Notification Queue names are a combination of a prefix plus a name NOTIFICATION_QUEUE_PREFIX = os.environ['NOTIFICATION_QUEUE_PREFIX'] - MMG_FROM_NUMBER = os.environ['MMG_FROM_NUMBER'] SECRET_KEY = os.environ['SECRET_KEY'] SQLALCHEMY_COMMIT_ON_TEARDOWN = False SQLALCHEMY_DATABASE_URI = os.environ['SQLALCHEMY_DATABASE_URI'] @@ -86,13 +85,12 @@ class Config(object): ] TWILIO_ACCOUNT_SID = os.getenv('TWILIO_ACCOUNT_SID') TWILIO_AUTH_TOKEN = os.getenv('TWILIO_AUTH_TOKEN') - TWILIO_NUMBER = os.getenv('TWILIO_NUMBER') - FIRETEXT_NUMBER = os.getenv('FIRETEXT_NUMBER') FIRETEXT_API_KEY = os.getenv("FIRETEXT_API_KEY") LOADTESTING_NUMBER = os.getenv('LOADTESTING_NUMBER') LOADTESTING_API_KEY = os.getenv("LOADTESTING_API_KEY") CSV_UPLOAD_BUCKET_NAME = 'local-notifications-csv-upload' NOTIFICATIONS_ALERT = 5 # five mins + FROM_NUMBER = os.getenv('FROM_NUMBER') STATSD_ENABLED = False STATSD_HOST = "localhost" diff --git a/config_live.py b/config_live.py index 35e53470b..5a5d1c973 100644 --- a/config_live.py +++ b/config_live.py @@ -14,7 +14,6 @@ class Live(Config): VERIFY_CODE_FROM_EMAIL_ADDRESS = os.environ['LIVE_VERIFY_CODE_FROM_EMAIL_ADDRESS'] NOTIFY_EMAIL_DOMAIN = os.environ['LIVE_NOTIFY_EMAIL_DOMAIN'] FIRETEXT_API_KEY = os.getenv("LIVE_FIRETEXT_API_KEY") - FIRETEXT_NUMBER = os.getenv("LIVE_FIRETEXT_NUMBER") TWILIO_AUTH_TOKEN = os.getenv('LIVE_TWILIO_AUTH_TOKEN') MMG_API_KEY = os.environ['LIVE_MMG_API_KEY'] CSV_UPLOAD_BUCKET_NAME = 'live-notifications-csv-upload' @@ -22,6 +21,7 @@ class Live(Config): STATSD_HOST = os.getenv('LIVE_STATSD_HOST') STATSD_PORT = os.getenv('LIVE_STATSD_PORT') STATSD_PREFIX = os.getenv('LIVE_STATSD_PREFIX') + FROM_NUMBER = os.getenv('LIVE_FROM_NUMBER') BROKER_TRANSPORT_OPTIONS = { 'region': 'eu-west-1', diff --git a/config_staging.py b/config_staging.py index 8deeb1eba..a2b46539b 100644 --- a/config_staging.py +++ b/config_staging.py @@ -14,10 +14,10 @@ class Staging(Config): VERIFY_CODE_FROM_EMAIL_ADDRESS = os.environ['STAGING_VERIFY_CODE_FROM_EMAIL_ADDRESS'] NOTIFY_EMAIL_DOMAIN = os.environ['STAGING_NOTIFY_EMAIL_DOMAIN'] FIRETEXT_API_KEY = os.getenv("STAGING_FIRETEXT_API_KEY") - FIRETEXT_NUMBER = os.getenv("STAGING_FIRETEXT_NUMBER") TWILIO_AUTH_TOKEN = os.getenv('STAGING_TWILIO_AUTH_TOKEN') MMG_API_KEY = os.environ['STAGING_MMG_API_KEY'] CSV_UPLOAD_BUCKET_NAME = 'staging-notifications-csv-upload' + FROM_NUMBER = os.getenv('STAGING_FROM_NUMBER') BROKER_TRANSPORT_OPTIONS = { 'region': 'eu-west-1', diff --git a/environment_test.sh b/environment_test.sh index 7c99e5d87..c03892e9e 100644 --- a/environment_test.sh +++ b/environment_test.sh @@ -14,12 +14,9 @@ export SQLALCHEMY_DATABASE_URI='postgresql://localhost/test_notification_api' export VERIFY_CODE_FROM_EMAIL_ADDRESS='no-reply@notify.works' export TWILIO_ACCOUNT_SID="test" export TWILIO_AUTH_TOKEN="test" -export TWILIO_NUMBER="test" export FIRETEXT_API_KEY="Firetext" -export FIRETEXT_NUMBER="Firetext" export NOTIFY_EMAIL_DOMAIN="test.notify.com" export MMG_API_KEY='mmg-secret-key' -export MMG_FROM_NUMBER='test' export LOADTESTING_API_KEY="loadtesting" export LOADTESTING_NUMBER="loadtesting" export STATSD_ENABLED=True @@ -27,3 +24,4 @@ export STATSD_HOST="localhost" export STATSD_PORT=1000 export STATSD_PREFIX="stats-prefix" export API_HOST_NAME="http://localhost:6011" +export FROM_NUMBER='from_number' diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 8dc6cae4b..28eeb2e19 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -547,7 +547,7 @@ def mock_firetext_client(mocker, statsd_client=None): statsd_client = statsd_client or mocker.Mock() current_app = mocker.Mock(config={ 'FIRETEXT_API_KEY': 'foo', - 'FIRETEXT_NUMBER': 'bar' + 'FROM_NUMBER': 'bar' }) client.init_app(current_app, statsd_client) return client @@ -559,7 +559,7 @@ def mock_mmg_client(mocker, statsd_client=None): statsd_client = statsd_client or mocker.Mock()() current_app = mocker.Mock(config={ 'MMG_API_KEY': 'foo', - 'MMG_FROM_NUMBER': 'bar' + 'FROM_NUMBER': 'bar' }) client.init_app(current_app, statsd_client) return client From 903c479d7afc991c6455f16f7cb9aaa6a9f4df80 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Tue, 7 Jun 2016 11:59:13 +0100 Subject: [PATCH 02/13] Update log messages so they don't produce an error on the utils logger. --- app/clients/sms/firetext.py | 2 +- app/clients/sms/mmg.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 2dfba89f0..b1982472c 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -93,7 +93,7 @@ class FiretextClient(SmsClient): "POST", "https://www.firetext.co.uk/api/sendsms", response.status_code, - firetext_response + firetext_response.items() ) ) except RequestException as e: diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 5d4698ba8..ac398aba7 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -91,7 +91,7 @@ class MMGClient(SmsClient): "POST", "https://www.mmgrp.co.uk/API/json/api.php", response.status_code, - response.json() + response.json().items() ) ) except RequestException as e: From e3d9dfad6ee78525d9897b3c409d888495faf358 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 7 Jun 2016 14:18:42 +0100 Subject: [PATCH 03/13] add template_statistics endpoint for specific template `/service//template-statistics/` still requires service-id just to try and keep api tree cleaner --- app/dao/notifications_dao.py | 6 ++ app/template_statistics/rest.py | 14 +++- tests/app/template_statistics/test_rest.py | 98 +++++++++++++++++++++- 3 files changed, 116 insertions(+), 2 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index ac9d597d9..d882512be 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -139,6 +139,12 @@ def dao_get_template_statistics_for_service(service_id, limit_days=None): desc(TemplateStatistics.updated_at)).all() +def dao_get_template_statistics_for_template(template_id): + return TemplateStatistics.query.filter( + TemplateStatistics.template_id == template_id + ).all() + + @transactional def dao_create_notification(notification, notification_type, provider_identifier): provider = ProviderDetails.query.filter_by(identifier=provider_identifier).one() diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 3446d2623..a2cbaebb2 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -5,7 +5,10 @@ from flask import ( current_app ) -from app.dao.notifications_dao import dao_get_template_statistics_for_service +from app.dao.notifications_dao import ( + dao_get_template_statistics_for_service, + dao_get_template_statistics_for_template +) from app.schemas import template_statistics_schema @@ -34,3 +37,12 @@ def get_template_statistics_for_service(service_id): if errors: return jsonify(result="error", message=errors), 400 return jsonify(data=data) + + +@template_statistics.route('/') +def get_template_statistics_for_template_id(service_id, template_id): + stats = dao_get_template_statistics_for_template(template_id) + data, errors = template_statistics_schema.dump(stats, many=True) + if errors: + return jsonify(result="error", message=errors), 400 + return jsonify(data=data) diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index a37506dd5..ef2a5faa7 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -1,10 +1,12 @@ +from datetime import datetime import json + from freezegun import freeze_time from app import db from app.models import TemplateStatistics - from tests import create_authorization_header +from tests.app.conftest import sample_template as create_sample_template @freeze_time('2016-04-09') @@ -130,3 +132,97 @@ def test_get_all_template_statistics_with_bad_limit_arg_returns_400(notify_api, json_resp = json.loads(response.get_data(as_text=True)) assert json_resp['result'] == 'error' assert json_resp['message'] == {'limit_days': ['blurk is not an integer']} + + +def test_get_template_statistics_for_template_only_returns_for_provided_template( + notify_db, + notify_db_session, + notify_api, + sample_service +): + template_1 = create_sample_template( + notify_db, + notify_db_session, + template_name='Sample Template 1', + service=sample_service + ) + template_2 = create_sample_template( + notify_db, + notify_db_session, + template_name='Sample Template 2', + service=sample_service + ) + + template_1_stats_1 = TemplateStatistics( + template_id=template_1.id, + service_id=sample_service.id, + day=datetime(2001, 1, 1) + ) + template_1_stats_2 = TemplateStatistics( + template_id=template_1.id, + service_id=sample_service.id, + day=datetime(2001, 1, 2) + ) + template_2_stats = TemplateStatistics( + template_id=template_2.id, + service_id=sample_service.id, + day=datetime(2001, 1, 1) + ) + db.session.add_all([template_1_stats_1, template_1_stats_2, template_2_stats]) + db.session.commit() + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/{}'.format(sample_service.id, template_1.id), + headers=[('Content-Type', 'application/json'), auth_header], + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['id'] == str(template_1_stats_1.id) + assert json_resp['data'][1]['id'] == str(template_1_stats_2.id) + + +def test_get_template_statistics_for_template_returns_empty_if_no_statistics( + notify_db, + notify_db_session, + notify_api, + sample_service +): + template_1 = create_sample_template( + notify_db, + notify_db_session, + template_name='Sample Template 1', + service=sample_service + ) + template_2 = create_sample_template( + notify_db, + notify_db_session, + template_name='Sample Template 2', + service=sample_service + ) + + template_1_stats = TemplateStatistics( + template_id=template_1.id, + service_id=sample_service.id, + day=datetime(2001, 1, 1) + ) + db.session.add(template_1_stats) + db.session.commit() + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + auth_header = create_authorization_header() + + response = client.get( + '/service/{}/template-statistics/{}'.format(sample_service.id, template_2.id), + headers=[('Content-Type', 'application/json'), auth_header], + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['data'] == [] From 03192b81c505802fc1a7ede3d02650614e0594a2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 7 Jun 2016 09:27:56 +0100 Subject: [PATCH 04/13] Make URLs appear as links in email clients Implements: https://github.com/alphagov/notifications-utils/pull/42 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 595a22d39..ce26322e4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,4 +22,4 @@ statsd==3.2.1 git+https://github.com/alphagov/notifications-python-client.git@1.0.0#egg=notifications-python-client==1.0.0 -git+https://github.com/alphagov/notifications-utils.git@6.2.0#egg=notifications-utils==6.2.0 +git+https://github.com/alphagov/notifications-utils.git@6.3.1#egg=notifications-utils==6.3.1 From e28ef237e432fed376b26d93c8819f8f23fc8451 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 7 Jun 2016 16:31:10 +0100 Subject: [PATCH 05/13] When adding a user new with permissions to a service, the permissions dao was deleting all permissions for that user (regardless of service id) as the last filter on the permissions dao get_query method won. I've added a replace flag to the set_user_service_permission method so that it can handle adding new users + permissions and editing of existing users' permissions. Also by pass the get_query method until it can be refactored to work correctly. For now execute the filter query directly on the model. --- app/dao/permissions_dao.py | 11 ++++--- app/service/rest.py | 21 ++++++------- app/user/rest.py | 2 +- tests/app/dao/test_services_dao.py | 50 ++++++++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 17 deletions(-) diff --git a/app/dao/permissions_dao.py b/app/dao/permissions_dao.py index 7ff9bb447..525b39a0f 100644 --- a/app/dao/permissions_dao.py +++ b/app/dao/permissions_dao.py @@ -32,6 +32,8 @@ class PermissionDAO(DAOClass): class Meta: model = Permission + # TODO rework this as last filter wins, whereas what is needed is + # append to filter so that semantics are 'and' def get_query(self, filter_by_dict=None): if filter_by_dict is None: filter_by_dict = MultiDict() @@ -60,13 +62,14 @@ class PermissionDAO(DAOClass): self.create_instance(permission, _commit=False) def remove_user_service_permissions(self, user, service): - query = self.get_query(filter_by_dict={'user': user.id, 'service': service.id}) + query = self.Meta.model.query.filter_by(user=user, service=service) query.delete() - def set_user_service_permission(self, user, service, permissions, _commit=False): + def set_user_service_permission(self, user, service, permissions, _commit=False, replace=False): try: - query = self.get_query(filter_by_dict={'user': user.id, 'service': service.id}) - query.delete() + if replace: + query = self.Meta.model.query.filter_by(user=user, service=service) + query.delete() for p in permissions: p.user = user p.service = service diff --git a/app/service/rest.py b/app/service/rest.py index 80b82fe67..0d2da9cb0 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -1,9 +1,13 @@ -from datetime import (datetime, date) +from datetime import ( + datetime, + date +) -from flask import Blueprint from flask import ( jsonify, - request + request, + abort, + Blueprint ) from sqlalchemy.orm.exc import NoResultFound @@ -152,6 +156,8 @@ def add_user_to_service(service_id, user_id): message='User id: {} already part of service id: {}'.format(user_id, service_id)), 400 permissions, errors = permission_schema.load(request.get_json(), many=True) + if errors: + abort(400, errors) dao_add_user_to_service(service, user, permissions) data, errors = service_schema.dump(service) @@ -174,15 +180,6 @@ def remove_user_from_service(service_id, user_id): return jsonify({}), 204 -def _process_permissions(user, service, permission_groups): - from app.permissions_utils import get_permissions_by_group - permissions = get_permissions_by_group(permission_groups) - for permission in permissions: - permission.user = user - permission.service = service - return permissions - - @service.route('//fragment/aggregate_statistics') def get_service_provider_aggregate_statistics(service_id): service = dao_fetch_service_by_id(service_id) diff --git a/app/user/rest.py b/app/user/rest.py index 1850c1c04..12fde343a 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -188,7 +188,7 @@ def set_permissions(user_id, service_id): for p in permissions: p.user = user p.service = service - permission_dao.set_user_service_permission(user, service, permissions, _commit=True) + permission_dao.set_user_service_permission(user, service, permissions, _commit=True, replace=True) return jsonify({}), 204 diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 199535d27..435d7aa35 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -334,3 +334,53 @@ def test_delete_service_and_associated_objects(notify_db, assert InvitedUser.query.count() == 0 assert Service.query.count() == 0 assert Service.get_history_model().query.count() == 0 + + +def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sample_user): + + service_one = Service(name="service_one", + email_from="service_one", + message_limit=1000, + active=True, + restricted=False, + created_by=sample_user) + + dao_create_service(service_one, sample_user) + assert sample_user.id == service_one.users[0].id + test_user_permissions = Permission.query.filter_by(service=service_one, user=sample_user).all() + assert len(test_user_permissions) == 8 + + other_user = User( + name='Other Test User', + email_address='other_user@digital.cabinet-office.gov.uk', + password='password', + mobile_number='+447700900987' + ) + save_model_user(other_user) + service_two = Service(name="service_two", + email_from="service_two", + message_limit=1000, + active=True, + restricted=False, + created_by=other_user) + dao_create_service(service_two, other_user) + + assert other_user.id == service_two.users[0].id + other_user_permissions = Permission.query.filter_by(service=service_two, user=other_user).all() + assert len(other_user_permissions) == 8 + + other_user_service_one_permissions = Permission.query.filter_by(service=service_one, user=other_user).all() + assert len(other_user_service_one_permissions) == 0 + + # adding the other_user to service_one should leave all other_user permissions on service_two intact + permissions = [] + for p in ['send_emails', 'send_texts', 'send_letters']: + permissions.append(Permission(permission=p)) + + dao_add_user_to_service(service_one, other_user, permissions=permissions) + + other_user_service_one_permissions = Permission.query.filter_by(service=service_one, user=other_user).all() + assert len(other_user_service_one_permissions) == 3 + + other_user_service_two_permissions = Permission.query.filter_by(service=service_two, user=other_user).all() + assert len(other_user_service_two_permissions) == 8 From bbd64b088bae74361fd8924adf5e43e8b4119fbe Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Wed, 8 Jun 2016 10:34:09 +0100 Subject: [PATCH 06/13] Added migration to add rate. Tested worked locally. --- .../versions/0027_update_provider_rates.py | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 migrations/versions/0027_update_provider_rates.py diff --git a/migrations/versions/0027_update_provider_rates.py b/migrations/versions/0027_update_provider_rates.py new file mode 100644 index 000000000..ebcc4bef2 --- /dev/null +++ b/migrations/versions/0027_update_provider_rates.py @@ -0,0 +1,35 @@ +"""empty message + +Revision ID: 0027_update_provider_rates +Revises: 0026_rename_notify_service +Create Date: 2016-06-08 01:00:00.000000 + +""" + +# revision identifiers, used by Alembic. +revision = '0027_update_provider_rates' +down_revision = '0026_rename_notify_service' + +from alembic import op +import sqlalchemy as sa +from datetime import datetime +import uuid + + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.get_bind() + op.execute(( + "INSERT INTO provider_rates (id, valid_from, rate, provider_id) VALUES ('{}', '{}', 1.8, " + "(SELECT id FROM provider_details WHERE identifier = 'mmg'))").format(uuid.uuid4(), datetime.utcnow())) + op.execute(( + "INSERT INTO provider_rates (id, valid_from, rate, provider_id) VALUES ('{}', '{}', 2.5, " + "(SELECT id FROM provider_details WHERE identifier = 'firetext'))").format(uuid.uuid4(), datetime.utcnow())) + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.get_bind() + op.execute("DELETE FROM provider_rates") + ### end Alembic commands ### From 8a1f4de2177419ae704c3e86dad161134d1a17a3 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Wed, 8 Jun 2016 15:25:57 +0100 Subject: [PATCH 07/13] Task added to update 'sending' notifications after 72 hours, set task to temporary-failure. --- app/celery/tasks.py | 27 ++++++++++++++++++++-- app/dao/notifications_dao.py | 4 ++++ config.py | 7 ++++++ tests/app/celery/test_tasks.py | 41 +++++++++++++++++++++++++++++++++- 4 files changed, 76 insertions(+), 3 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 80925b5af..28eae3556 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,10 +1,11 @@ import itertools -from datetime import datetime +from datetime import (datetime, timedelta) from flask import current_app from monotonic import monotonic from sqlalchemy.exc import SQLAlchemyError from app import clients, statsd_client +from app.clients import STATISTICS_FAILURE from app.clients.email import EmailClientException from app.clients.sms import SmsClientException from app.dao.services_dao import dao_fetch_service_by_id @@ -37,7 +38,9 @@ from app.dao.notifications_dao import ( dao_update_notification, delete_notifications_created_more_than_a_week_ago, dao_get_notification_statistics_for_service_and_day, - update_provider_stats + update_provider_stats, + get_notifications, + update_notification_status_by_id ) from app.dao.jobs_dao import ( @@ -524,3 +527,23 @@ def provider_to_use(notification_type, notification_id): raise Exception("No active {} providers".format(notification_type)) return clients.get_client_by_name_and_type(active_providers_in_order[0].identifier, notification_type) + + +@notify_celery.task(name='timeout-sending-notifications') +def timeout_notifications(): + notifications = get_notifications(filter_dict={'status': 'sending'}) + now = datetime.utcnow() + for noti in notifications: + try: + if (now - noti.created_at) > timedelta( + seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + ): + update_notification_status_by_id(noti.id, 'temporary-failure', STATISTICS_FAILURE) + current_app.logger.info(( + "Timeout period reached for notification ({})" + ", status has been updated.").format(noti.id)) + except Exception as e: + current_app.logger.exception(e) + current_app.logger.error(( + "Exception raised trying to timeout notification ({})" + ", skipping notification update.").format(noti.id)) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d882512be..fa911e2b2 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -338,6 +338,10 @@ def get_notification_by_id(notification_id): return Notification.query.filter_by(id=notification_id).first() +def get_notifications(filter_dict=None): + return _filter_query(Notification.query, filter_dict=filter_dict) + + def get_notifications_for_service(service_id, filter_dict=None, page=1, diff --git a/config.py b/config.py index 69b0e6694..35a841bbf 100644 --- a/config.py +++ b/config.py @@ -67,6 +67,11 @@ class Config(object): 'task': 'delete-successful-notifications', 'schedule': crontab(minute=0, hour='0,1,2'), 'options': {'queue': 'periodic'} + }, + 'timeout-sending-notifications': { + 'task': 'timeout-sending-notifications', + 'schedule': crontab(minute=0, hour='0,1,2'), + 'options': {'queue': 'periodic'} } } CELERY_QUEUES = [ @@ -99,6 +104,8 @@ class Config(object): STATSD_PORT = None STATSD_PREFIX = None + SENDING_NOTIFICATIONS_TIMEOUT_PERIOD = 259200 + class Development(Config): DEBUG = True diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index e0b8c7166..c2077d97e 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -15,7 +15,8 @@ from app.celery.tasks import ( delete_invitations, delete_failed_notifications, delete_successful_notifications, - provider_to_use + provider_to_use, + timeout_notifications ) from app.celery.research_mode_tasks import ( send_email_response, @@ -1184,3 +1185,41 @@ def _notification_json(template, to, personalisation=None, job_id=None, row_numb if row_number: notification['row_number'] = row_number return notification + + +def test_update_status_of_notifications_after_timeout(notify_api, + notify_db, + notify_db_session, + sample_service, + sample_template, + mmg_provider): + with notify_api.test_request_context(): + not1 = sample_notification( + notify_db, + notify_db_session, + service=sample_service, + template=sample_template, + status='sending', + created_at=datetime.utcnow() - timedelta( + seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10)) + timeout_notifications() + assert not1.status == 'temporary-failure' + + +def test_not_update_status_of_notification_before_timeout(notify_api, + notify_db, + notify_db_session, + sample_service, + sample_template, + mmg_provider): + with notify_api.test_request_context(): + not1 = sample_notification( + notify_db, + notify_db_session, + service=sample_service, + template=sample_template, + status='sending', + created_at=datetime.utcnow() - timedelta( + seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') - 10)) + timeout_notifications() + assert not1.status == 'sending' From 6926769f6683b91a650c8fd6d8acd630d018deb5 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Jun 2016 10:25:27 +0100 Subject: [PATCH 08/13] log firetext response code --- app/notifications/rest.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index d1be71474..3b7541354 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -176,9 +176,12 @@ def process_firetext_response(): current_app.logger.info(validation_errors) return jsonify(result='error', message=validation_errors), 400 - statsd_client.incr('notifications.callback.firetext.code.{}'.format(request.form.get('code'))) + response_code = request.form.get('code') + status = request.form.get('status') + statsd_client.incr('notifications.callback.firetext.code.{}'.format(response_code)) + current_app.logger.info('Firetext status: {}, extended error code: {}'.format(status, response_code)) - success, errors = process_sms_client_response(status=request.form.get('status'), + success, errors = process_sms_client_response(status=status, reference=request.form.get('reference'), client_name=client_name) if errors: From 3e72440f38baa8632624bb818e7b2175ae6bebe1 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Jun 2016 11:21:48 +0100 Subject: [PATCH 09/13] fix template api tests being inconsistent by adding ordering --- app/dao/notifications_dao.py | 2 ++ tests/app/template_statistics/test_rest.py | 11 ++++++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index fa911e2b2..2ea459267 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -142,6 +142,8 @@ def dao_get_template_statistics_for_service(service_id, limit_days=None): def dao_get_template_statistics_for_template(template_id): return TemplateStatistics.query.filter( TemplateStatistics.template_id == template_id + ).order_by( + desc(TemplateStatistics.updated_at) ).all() diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index ef2a5faa7..4d7b3384a 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -168,7 +168,12 @@ def test_get_template_statistics_for_template_only_returns_for_provided_template service_id=sample_service.id, day=datetime(2001, 1, 1) ) - db.session.add_all([template_1_stats_1, template_1_stats_2, template_2_stats]) + + # separate commit to ensure stats_1 has earlier updated_at time + db.session.add(template_1_stats_1) + db.session.commit() + + db.session.add_all([template_1_stats_2, template_2_stats]) db.session.commit() with notify_api.test_request_context(): @@ -183,8 +188,8 @@ def test_get_template_statistics_for_template_only_returns_for_provided_template assert response.status_code == 200 json_resp = json.loads(response.get_data(as_text=True)) assert len(json_resp['data']) == 2 - assert json_resp['data'][0]['id'] == str(template_1_stats_1.id) - assert json_resp['data'][1]['id'] == str(template_1_stats_2.id) + assert json_resp['data'][0]['id'] == str(template_1_stats_2.id) + assert json_resp['data'][1]['id'] == str(template_1_stats_1.id) def test_get_template_statistics_for_template_returns_empty_if_no_statistics( From 50ec2eb7fc046c143c86e74e37688061ed10b3ed Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 9 Jun 2016 11:54:34 +0100 Subject: [PATCH 10/13] Remove task send_sms_code. We use a notify service to send the sms code via using a template. --- app/celery/tasks.py | 14 -------------- tests/app/celery/test_tasks.py | 28 ---------------------------- 2 files changed, 42 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 28eae3556..d3ccbdfc4 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -375,20 +375,6 @@ def send_email(service_id, notification_id, from_address, encrypted_notification current_app.logger.exception(e) -@notify_celery.task(name='send-sms-code') -def send_sms_code(encrypted_verification): - provider = provider_to_use('sms', 'send-sms-code') - - verification_message = encryption.decrypt(encrypted_verification) - try: - provider.send_sms(validate_and_format_phone_number(verification_message['to']), - "{} is your Notify authentication code".format( - verification_message['secret_code']), - 'send-sms-code') - except SmsClientException as e: - current_app.logger.exception(e) - - # TODO: when placeholders in templates work, this will be a real template def invitation_template(user_name, service_name, url, expiry_date): from string import Template diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index c2077d97e..7034368d8 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -6,7 +6,6 @@ from notifications_utils.recipients import validate_phone_number, format_phone_n from app.celery.tasks import ( send_sms, - send_sms_code, send_email, process_job, email_invited_user, @@ -898,33 +897,6 @@ def test_should_not_send_email_if_db_peristance_failed(sample_email_template, mo assert 'No row was found for one' in str(e.value) -def test_should_send_sms_code(mocker): - notification = {'to': '+447234123123', - 'secret_code': '12345'} - - encrypted_notification = encryption.encrypt(notification) - - mocker.patch('app.mmg_client.send_sms') - send_sms_code(encrypted_notification) - mmg_client.send_sms.assert_called_once_with( - format_phone_number(validate_phone_number(notification['to'])), - "{} is your Notify authentication code".format(notification['secret_code']), - 'send-sms-code') - - -def test_should_throw_mmg_client_exception(mocker): - notification = {'to': '+447234123123', - 'secret_code': '12345'} - - encrypted_notification = encryption.encrypt(notification) - mocker.patch('app.mmg_client.send_sms', side_effect=MMGClientException(mmg_error)) - send_sms_code(encrypted_notification) - mmg_client.send_sms.assert_called_once_with( - format_phone_number(validate_phone_number(notification['to'])), - "{} is your Notify authentication code".format(notification['secret_code']), - 'send-sms-code') - - def test_email_invited_user_should_send_email(notify_api, mocker): with notify_api.test_request_context(): invitation = {'to': 'new_person@it.gov.uk', From fd2a0a0b100534c1f4bd30cddbc18cd07f64bd87 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Jun 2016 14:14:27 +0100 Subject: [PATCH 11/13] fix firetext research mode requests requests converts dicts into query parameters anyway if you don't specify so dont try and encode the data ourselves, also hardened up tests --- app/celery/research_mode_tasks.py | 9 +++++++-- tests/app/celery/test_research_mode_tasks.py | 18 ++++++++++++++++-- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/app/celery/research_mode_tasks.py b/app/celery/research_mode_tasks.py index 907716706..0b0a5d4e4 100644 --- a/app/celery/research_mode_tasks.py +++ b/app/celery/research_mode_tasks.py @@ -19,7 +19,7 @@ def send_sms_response(provider, reference, to): body = mmg_callback(reference, to) headers = {"Content-type": "application/json"} else: - headers = {"Content-type": "text/plain"} + headers = {"Content-type": "application/x-www-form-urlencoded"} body = firetext_callback(reference, to) make_request('sms', provider, body, headers) @@ -92,7 +92,12 @@ def firetext_callback(notification_id, to): status = "1" else: status = "0" - return 'mobile={}&status={}&time=2016-03-10 14:17:00&reference={}'.format(to, status, notification_id) + return { + 'mobile': to, + 'status': status, + 'time': '2016-03-10 14:17:00', + 'reference': notification_id + } def ses_notification_callback(reference): diff --git a/tests/app/celery/test_research_mode_tasks.py b/tests/app/celery/test_research_mode_tasks.py index 8c99e98c5..9aaf33329 100644 --- a/tests/app/celery/test_research_mode_tasks.py +++ b/tests/app/celery/test_research_mode_tasks.py @@ -20,6 +20,8 @@ def test_make_mmg_callback(notify_api, rmock): send_sms_response("mmg", "1234", "07811111111") assert rmock.called + assert rmock.request_history[0].url == endpoint + assert json.loads(rmock.request_history[0].text)['MSISDN'] == '07811111111' def test_make_firetext_callback(notify_api, rmock): @@ -32,6 +34,8 @@ def test_make_firetext_callback(notify_api, rmock): send_sms_response("firetext", "1234", "07811111111") assert rmock.called + assert rmock.request_history[0].url == endpoint + assert 'mobile=07811111111' in rmock.request_history[0].text def test_make_ses_callback(notify_api, rmock): @@ -71,11 +75,21 @@ def test_temp_failure_mmg_callback(): def test_delivered_firetext_callback(): - assert firetext_callback("1234", "07811111111") == "mobile=07811111111&status=0&time=2016-03-10 14:17:00&reference=1234" # noqa + assert firetext_callback('1234', '07811111111') == { + 'mobile': '07811111111', + 'status': '0', + 'time': '2016-03-10 14:17:00', + 'reference': '1234' + } def test_failure_firetext_callback(): - assert firetext_callback("1234", "07822222222") == "mobile=07822222222&status=1&time=2016-03-10 14:17:00&reference=1234" # noqa + assert firetext_callback('1234', '07822222222') == { + 'mobile': '07822222222', + 'status': '1', + 'time': '2016-03-10 14:17:00', + 'reference': '1234' + } def test_delivered_ses_callback(): From 7083be429a64d8aa0de47a760cc06ecbb0a9860f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 10 Jun 2016 15:37:05 +0100 Subject: [PATCH 12/13] exclude notifications from job was being lazily loaded to get notification ids, so every time a job is loaded it would potentially select thousands of notifications from the database --- app/schemas.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/schemas.py b/app/schemas.py index 9c9406a15..7d20012cf 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -160,6 +160,7 @@ class JobSchema(BaseSchema): class Meta: model = models.Job + exclude = ('notifications',) class RequestVerifyCodeSchema(ma.Schema): From edc72a57ca8b8a20e13dd2f2c95afbf88eb5b9d6 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 10 Jun 2016 15:54:21 +0100 Subject: [PATCH 13/13] add test file for schemas --- tests/app/test_schemas.py | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 tests/app/test_schemas.py diff --git a/tests/app/test_schemas.py b/tests/app/test_schemas.py new file mode 100644 index 000000000..73d35002a --- /dev/null +++ b/tests/app/test_schemas.py @@ -0,0 +1,10 @@ +def test_job_schema_doesnt_return_notifications(sample_notification): + from app.schemas import job_schema + + job = sample_notification.job + assert job.notifications.count() == 1 + + data, errors = job_schema.dump(job) + + assert not errors + assert 'notifications' not in data