From 700e3d2fa775031dac262b884cf99130ce8cef3a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 12 May 2017 14:59:14 +0100 Subject: [PATCH 01/42] update delivery receipts for countries that return them some countries don't return delivery receipts some countries return delivery receipts when they reach the carrier these countries, we should keep the notifications in sent (aka sent_internatinally) for. However, for countries that have normal delivery receipts, we should update them as we do for UK numbers --- app/dao/notifications_dao.py | 10 +++++- tests/app/dao/test_notification_dao.py | 44 ++++++++++++++++++-------- 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index da81c348b..a81b6cad5 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -8,6 +8,7 @@ from flask import current_app from werkzeug.datastructures import MultiDict from sqlalchemy import (desc, func, or_, and_, asc) from sqlalchemy.orm import joinedload +from notifications_utils.international_billing_rates import INTERNATIONAL_BILLING_RATES from app import db, create_uuid from app.dao import days_ago @@ -163,6 +164,10 @@ def _decide_permanent_temporary_failure(current_status, status): return status +def country_records_delivery(phone_prefix): + return INTERNATIONAL_BILLING_RATES[phone_prefix]['attributes']['dlr'].lower() == 'yes' + + def _update_notification_status(notification, status): status = _decide_permanent_temporary_failure(current_status=notification.status, status=status) notification.status = status @@ -182,7 +187,10 @@ def update_notification_status_by_id(notification_id, status): Notification.status == NOTIFICATION_SENT )).first() - if not notification or notification.status == NOTIFICATION_SENT: + if not notification: + return None + + if notification.international and not country_records_delivery(notification.phone_prefix): return None return _update_notification_status( diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 95dc3e09d..c6c8d2b0b 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -15,6 +15,7 @@ from app.models import ( NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_TYPES_FAILED, NOTIFICATION_SENT, + NOTIFICATION_DELIVERED, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST) @@ -353,28 +354,45 @@ def test_should_update_status_by_id_if_created(notify_db, notify_db_session): assert updated.status == 'failed' -def test_should_not_update_status_by_reference_if_in_sent_status(notify_db, notify_db_session): - notification = sample_notification( - notify_db, - notify_db_session, +def test_should_not_update_status_by_reference_if_from_country_with_no_delivery_receipts(sample_template): + notification = create_notification( + sample_template, status=NOTIFICATION_SENT, reference='foo' ) - update_notification_status_by_reference('foo', 'failed') - assert Notification.query.get(notification.id).status == NOTIFICATION_SENT + res = update_notification_status_by_reference('foo', 'failed') + + assert res is None + assert notification.status == NOTIFICATION_SENT -def test_should_not_update_status_by_id_if_in_sent_status(notify_db, notify_db_session): - notification = sample_notification( - notify_db, - notify_db_session, - status=NOTIFICATION_SENT +def test_should_not_update_status_by_id_if_sent_to_country_with_no_delivery_receipts(sample_template): + notification = create_notification( + sample_template, + status=NOTIFICATION_SENT, + international=True, + phone_prefix='1' # americans only have carrier delivery receipts ) - update_notification_status_by_id(notification.id, 'failed') + res = update_notification_status_by_id(notification.id, 'delivered') - assert Notification.query.get(notification.id).status == NOTIFICATION_SENT + assert res is None + assert notification.status == NOTIFICATION_SENT + + +def test_should_not_update_status_by_id_if_sent_to_country_with_no_delivery_receipts(sample_template): + notification = create_notification( + sample_template, + status=NOTIFICATION_SENT, + international=True, + phone_prefix='7' # russians have full delivery receipts + ) + + res = update_notification_status_by_id(notification.id, 'delivered') + + assert res == notification + assert notification.status == NOTIFICATION_DELIVERED def test_should_not_update_status_by_reference_if_not_sending(notify_db, notify_db_session): From 75a4dad8c1cb1a9f37f5e3d7017009e7f567162b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 15 May 2017 12:59:44 +0100 Subject: [PATCH 02/42] New table to hold scheduled_notifications. --- app/models.py | 10 ++++++ .../versions/0083_scheduled_notifications.py | 31 +++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100644 migrations/versions/0083_scheduled_notifications.py diff --git a/app/models.py b/app/models.py index 1b07a6fc5..1fe07460e 100644 --- a/app/models.py +++ b/app/models.py @@ -912,6 +912,16 @@ class NotificationHistory(db.Model, HistoryModel): INVITED_USER_STATUS_TYPES = ['pending', 'accepted', 'cancelled'] +class ScheduledNotification(db.Model): + __tablename__ = 'scheduled_notifications' + + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4()) + notification_id = db.Column(UUID(as_uuid=True), db.ForeignKey('notifications.id'), index=True, nullable=False) + notification = db.relationship('Notification') + scheduled_for = db.Column(db.DateTime, index=False, nullable=False) + pending = db.Column(db.Boolean, nullable=False, default=False) + + class InvitedUser(db.Model): __tablename__ = 'invited_users' diff --git a/migrations/versions/0083_scheduled_notifications.py b/migrations/versions/0083_scheduled_notifications.py new file mode 100644 index 000000000..3e1d295e6 --- /dev/null +++ b/migrations/versions/0083_scheduled_notifications.py @@ -0,0 +1,31 @@ +"""empty message + +Revision ID: 0083_scheduled_notifications +Revises: 0082_add_go_live_template +Create Date: 2017-05-15 12:50:20.041950 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0083_scheduled_notifications' +down_revision = '0082_add_go_live_template' + + +def upgrade(): + op.create_table('scheduled_notifications', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('notification_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('scheduled_for', sa.DateTime(), nullable=False), + sa.Column('pending', sa.Boolean(), nullable=False), + sa.ForeignKeyConstraint(['notification_id'], ['notifications.id'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_index(op.f('ix_scheduled_notifications_notification_id'), 'scheduled_notifications', ['notification_id'], + unique=False) + + +def downgrade(): + op.drop_index(op.f('ix_scheduled_notifications_notification_id'), table_name='scheduled_notifications') + op.drop_table('scheduled_notifications') From 38e5b31e9accb368e28c6011b1591598fcaec9ec Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 15 May 2017 15:02:38 +0100 Subject: [PATCH 03/42] Update notification schemas with optional schedule_for element --- app/schema_validation/__init__.py | 11 +++++ app/v2/notifications/notification_schemas.py | 15 ++++--- .../test_notification_schemas.py | 42 +++++++++++++++++-- 3 files changed, 60 insertions(+), 8 deletions(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 376315a8a..fc134e2ce 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -1,4 +1,5 @@ import json +from datetime import datetime from jsonschema import (Draft4Validator, ValidationError, FormatChecker) from notifications_utils.recipients import (validate_phone_number, validate_email_address, InvalidPhoneError, @@ -20,6 +21,16 @@ def validate(json_to_validate, schema): validate_email_address(instance) return True + @format_checker.checks('datetime', raises=ValidationError) + def validate_schema_datetime(instance): + if isinstance(instance, str): + try: + datetime.strptime(instance, "%Y-%m-%d %H:%M:%S") + except ValueError as e: + raise ValidationError("datetime format is invalid. Use the format: " + "YYYY-MM-DD HH:MM:SS, for example 2017-05-30 13:00:00") + return True + validator = Draft4Validator(schema, format_checker=format_checker) errors = list(validator.iter_errors(json_to_validate)) if errors.__len__() > 0: diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 240d41324..8777b49ba 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -39,7 +39,8 @@ get_notification_response = { "subject": {"type": ["string", "null"]}, "created_at": {"type": "string"}, "sent_at": {"type": ["string", "null"]}, - "completed_at": {"type": ["string", "null"]} + "completed_at": {"type": ["string", "null"]}, + "scheduled_for": {"type": ["string", "null"]} }, "required": [ # technically, all keys are required since we always have all of them @@ -111,7 +112,8 @@ post_sms_request = { "reference": {"type": "string"}, "phone_number": {"type": "string", "format": "phone_number"}, "template_id": uuid, - "personalisation": personalisation + "personalisation": personalisation, + "scheduled_for": {"type": "string", "format": "datetime"} }, "required": ["phone_number", "template_id"] } @@ -138,7 +140,8 @@ post_sms_response = { "reference": {"type": ["string", "null"]}, "content": sms_content, "uri": {"type": "string", "format": "uri"}, - "template": template + "template": template, + "scheduled_for": {"type": "string"} }, "required": ["id", "content", "uri", "template"] } @@ -153,7 +156,8 @@ post_email_request = { "reference": {"type": "string"}, "email_address": {"type": "string", "format": "email_address"}, "template_id": uuid, - "personalisation": personalisation + "personalisation": personalisation, + "scheduled_for": {"type": "string", "format": "datetime"} }, "required": ["email_address", "template_id"] } @@ -181,7 +185,8 @@ post_email_response = { "reference": {"type": ["string", "null"]}, "content": email_content, "uri": {"type": "string", "format": "uri"}, - "template": template + "template": template, + "scheduled_for": {"type": "string"} }, "required": ["id", "content", "uri", "template"] } diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 65a03b84d..44794f9de 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -246,7 +246,8 @@ def valid_email_response(): "id": str(uuid.uuid4()), "version": 1, "uri": "http://notify.api/v2/template/id" - } + }, + "scheduled_for": "" } @@ -262,7 +263,8 @@ def valid_email_response_with_optionals(): "id": str(uuid.uuid4()), "version": 1, "uri": "http://notify.api/v2/template/id" - } + }, + "schedule_for": "2017-05-12 13:00:00" } @@ -346,7 +348,41 @@ def test_get_notifications_response_with_email_and_phone_number(): "subject": "some subject", "created_at": "2016-01-01", "sent_at": "2016-01-01", - "completed_at": "2016-01-01" + "completed_at": "2016-01-01", + "schedule_for": "" } assert validate(response, get_notification_response) == response + + +@pytest.mark.parametrize("schema", + [post_email_request_schema, post_sms_request_schema]) +def test_post_schema_valid_scheduled_for(schema): + j = {"template_id": str(uuid.uuid4()), + "email_address": "joe@gmail.com", + "scheduled_for": "2017-05-12 13:00:00"} + if schema == post_email_request_schema: + j.update({"email_address": "joe@gmail.com"}) + else: + j.update({"phone_number": "07515111111"}) + assert validate(j, schema) == j + + +@pytest.mark.parametrize("invalid_datetime", + ["2017-05-12 13:00", "13:00:00 2017-01-01"]) +@pytest.mark.parametrize("schema", + [post_email_request_schema, post_sms_request_schema]) +def test_post_email_schema_invalid_scheduled_for(invalid_datetime, schema): + j = {"template_id": str(uuid.uuid4()), + "scheduled_for": invalid_datetime} + if schema == post_email_request_schema: + j.update({"email_address": "joe@gmail.com"}) + else: + j.update({"phone_number": "07515111111"}) + with pytest.raises(ValidationError) as e: + validate(j, schema) + error = json.loads(str(e.value)) + assert error['status_code'] == 400 + assert error['errors'] == [{'error': 'ValidationError', + 'message': "scheduled_for datetime format is invalid. Use the format: " + "YYYY-MM-DD HH:MM:SS, for example 2017-05-30 13:00:00"}] From f0e2713bef87403899b4eef98c92a0d287281708 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 15 May 2017 17:27:38 +0100 Subject: [PATCH 04/42] Add scheduled_for in the post notification request form. Return scheduled for in get_notification requests. --- app/dao/notifications_dao.py | 6 ++ app/models.py | 6 +- app/notifications/process_notifications.py | 12 ++- app/v2/notifications/notification_schemas.py | 13 +++- app/v2/notifications/post_notifications.py | 23 ++++-- tests/app/conftest.py | 13 +++- tests/app/dao/test_notification_dao.py | 15 +++- .../notifications/test_get_notifications.py | 35 ++++++++- .../notifications/test_post_notifications.py | 74 +++++++++++++------ 9 files changed, 151 insertions(+), 46 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index da81c348b..1a97418d3 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -463,3 +463,9 @@ def dao_get_notifications_by_to_field(service_id, search_term): return Notification.query.filter( Notification.service_id == service_id, func.replace(func.lower(Notification.to), " ", "") == search_term.lower().replace(" ", "")).all() + + +@statsd(namespace="dao") +def dao_created_scheduled_notification(scheduled_notification): + db.session.add(scheduled_notification) + db.session.commit() diff --git a/app/models.py b/app/models.py index 1fe07460e..f15507e01 100644 --- a/app/models.py +++ b/app/models.py @@ -686,6 +686,8 @@ class Notification(db.Model): foreign(template_version) == remote(TemplateHistory.version) )) + scheduled_for = db.relationship('ScheduledNotification') + client_reference = db.Column(db.String, index=True, nullable=True) international = db.Column(db.Boolean, nullable=False, default=False) @@ -846,7 +848,9 @@ class Notification(db.Model): "subject": self.subject, "created_at": self.created_at.strftime(DATETIME_FORMAT), "sent_at": self.sent_at.strftime(DATETIME_FORMAT) if self.sent_at else None, - "completed_at": self.completed_at() + "completed_at": self.completed_at(), + "scheduled_for": self.scheduled_for[0].scheduled_for.strftime( + DATETIME_FORMAT) if self.scheduled_for else None } return serialized diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index e8510ba63..bb88f1265 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -10,8 +10,10 @@ from notifications_utils.recipients import ( from app import redis_store from app.celery import provider_tasks from notifications_utils.clients import redis -from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id -from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE +from app.dao.notifications_dao import (dao_create_notification, + dao_delete_notifications_and_history_by_id, + dao_created_scheduled_notification) +from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE, ScheduledNotification from app.v2.errors import BadRequestError, SendNotificationToQueueError from app.utils import get_template_instance, cache_key_for_service_template_counter @@ -120,3 +122,9 @@ def simulated_recipient(to_address, notification_type): return to_address in formatted_simulated_numbers else: return to_address in current_app.config['SIMULATED_EMAIL_ADDRESSES'] + + +def persist_scheduled_notification(notification_id, scheduled_for): + scheduled_notification = ScheduledNotification(notification_id=notification_id, + scheduled_for=scheduled_for) + dao_created_scheduled_notification(scheduled_notification) diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 8777b49ba..c4752ef7b 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -1,3 +1,5 @@ +from datetime import datetime + from app.models import NOTIFICATION_STATUS_TYPES, TEMPLATE_TYPES from app.schema_validation.definitions import (uuid, personalisation) @@ -192,7 +194,7 @@ post_email_response = { } -def create_post_sms_response_from_notification(notification, body, from_number, url_root, service_id): +def create_post_sms_response_from_notification(notification, body, from_number, url_root, service_id, scheduled_for): return {"id": notification.id, "reference": notification.client_reference, "content": {'body': body, @@ -200,11 +202,13 @@ def create_post_sms_response_from_notification(notification, body, from_number, "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)), "template": __create_template_from_notification(notification=notification, url_root=url_root, - service_id=service_id) + service_id=service_id), + "scheduled_for": scheduled_for } -def create_post_email_response_from_notification(notification, content, subject, email_from, url_root, service_id): +def create_post_email_response_from_notification(notification, content, subject, email_from, url_root, service_id, + scheduled_for): return { "id": notification.id, "reference": notification.client_reference, @@ -216,7 +220,8 @@ def create_post_email_response_from_notification(notification, content, subject, "uri": "{}v2/notifications/{}".format(url_root, str(notification.id)), "template": __create_template_from_notification(notification=notification, url_root=url_root, - service_id=service_id) + service_id=service_id), + "scheduled_for": scheduled_for } diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 347884ed8..a7f666a99 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -8,7 +8,8 @@ from app.notifications.process_notifications import ( create_content_for_notification, persist_notification, send_notification_to_queue, - simulated_recipient) + simulated_recipient, + persist_scheduled_notification) from app.notifications.validators import ( check_template_is_for_notification_type, check_template_is_active, @@ -57,26 +58,32 @@ def post_notification(notification_type): key_type=api_user.key_type, client_reference=form.get('reference', None), simulated=simulated) - - if not simulated: - queue_name = 'priority' if template.process_type == PRIORITY else None - send_notification_to_queue(notification=notification, research_mode=service.research_mode, queue=queue_name) + scheduled_for = form.get("scheduled_for", None) + if scheduled_for: + persist_scheduled_notification(notification.id, form["scheduled_for"]) else: - current_app.logger.info("POST simulated notification for id: {}".format(notification.id)) + if not simulated: + queue_name = 'priority' if template.process_type == PRIORITY else None + send_notification_to_queue(notification=notification, research_mode=service.research_mode, queue=queue_name) + else: + current_app.logger.info("POST simulated notification for id: {}".format(notification.id)) + if notification_type == SMS_TYPE: sms_sender = service.sms_sender if service.sms_sender else current_app.config.get('FROM_NUMBER') resp = create_post_sms_response_from_notification(notification=notification, body=str(template_with_content), from_number=sms_sender, url_root=request.url_root, - service_id=service.id) + service_id=service.id, + scheduled_for=scheduled_for) else: resp = create_post_email_response_from_notification(notification=notification, content=str(template_with_content), subject=template_with_content.subject, email_from=service.email_from, url_root=request.url_root, - service_id=service.id) + service_id=service.id, + scheduled_for=scheduled_for) return jsonify(resp), 201 diff --git a/tests/app/conftest.py b/tests/app/conftest.py index fc9748870..7e859324f 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -24,7 +24,7 @@ from app.models import ( NotificationStatistics, ServiceWhitelist, KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM, - MOBILE_TYPE, EMAIL_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED) + MOBILE_TYPE, EMAIL_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED, ScheduledNotification) from app.dao.users_dao import (create_user_code, create_secret_code) from app.dao.services_dao import (dao_create_service, dao_add_user_to_service) from app.dao.templates_dao import dao_create_template @@ -445,7 +445,8 @@ def sample_notification( key_type=KEY_TYPE_NORMAL, sent_by=None, client_reference=None, - rate_multiplier=1.0 + rate_multiplier=1.0, + scheduled_for=None ): if created_at is None: created_at = datetime.utcnow() @@ -489,6 +490,14 @@ def sample_notification( data['job_row_number'] = job_row_number notification = Notification(**data) dao_create_notification(notification) + if scheduled_for: + scheduled_notification = ScheduledNotification(id=uuid.uuid4(), + notification_id=notification.id, + scheduled_for=datetime.strptime(scheduled_for, + "%Y-%m-%d %H:%M:%S")) + db.session.add(scheduled_notification) + db.session.commit() + return notification diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 95dc3e09d..0f1f3721c 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -12,6 +12,7 @@ from app.models import ( Job, NotificationStatistics, TemplateStatistics, + ScheduledNotification, NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_TYPES_FAILED, NOTIFICATION_SENT, @@ -39,7 +40,9 @@ from app.dao.notifications_dao import ( dao_delete_notifications_and_history_by_id, dao_timeout_notifications, is_delivery_slow_for_provider, - dao_update_notifications_sent_to_dvla, dao_get_notifications_by_to_field) + dao_update_notifications_sent_to_dvla, + dao_get_notifications_by_to_field, + dao_created_scheduled_notification) from app.dao.services_dao import dao_update_service from tests.app.db import create_notification @@ -1691,3 +1694,13 @@ def test_dao_get_notifications_by_to_field_search_ignores_spaces(sample_template assert notification1.id in [r.id for r in results] assert notification2.id in [r.id for r in results] assert notification3.id in [r.id for r in results] + + +def test_dao_created_scheduled_notification(sample_notification): + scheduled_notification = ScheduledNotification(notification_id=sample_notification.id, + scheduled_for="2017-01-05 14:00:00") + dao_created_scheduled_notification(scheduled_notification) + saved_notification = ScheduledNotification.query.all() + assert len(saved_notification) == 1 + assert saved_notification[0].notification_id == sample_notification.id + assert saved_notification[0].scheduled_for == datetime(2017, 1, 5, 14) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 6e3b72ed2..2b8fd0832 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -16,12 +16,17 @@ from tests.app.conftest import ( (1, None) ]) def test_get_notification_by_id_returns_200( - client, notify_db, notify_db_session, sample_provider_rate, billable_units, provider + client, notify_db, notify_db_session, billable_units, provider ): sample_notification = create_sample_notification( - notify_db, notify_db_session, billable_units=billable_units, sent_by=provider + notify_db, notify_db_session, billable_units=billable_units, sent_by=provider, + scheduled_for="2017-05-12 14:00:00" ) + another = create_sample_notification( + notify_db, notify_db_session, billable_units=billable_units, sent_by=provider, + scheduled_for="2017-06-12 14:00:00" + ) auth_header = create_authorization_header(service_id=sample_notification.service_id) response = client.get( path='/v2/notifications/{}'.format(sample_notification.id), @@ -57,7 +62,8 @@ def test_get_notification_by_id_returns_200( 'body': sample_notification.template.content, "subject": None, 'sent_at': sample_notification.sent_at, - 'completed_at': sample_notification.completed_at() + 'completed_at': sample_notification.completed_at(), + 'scheduled_for': '2017-05-12T14:00:00.000000Z' } assert json_response == expected_response @@ -105,7 +111,8 @@ def test_get_notification_by_id_with_placeholders_returns_200( 'body': "Hello Bob\nThis is an email from GOV.\u200bUK", "subject": "Bob", 'sent_at': sample_notification.sent_at, - 'completed_at': sample_notification.completed_at() + 'completed_at': sample_notification.completed_at(), + 'scheduled_for': None } assert json_response == expected_response @@ -130,6 +137,25 @@ def test_get_notification_by_reference_returns_200(client, notify_db, notify_db_ assert json_response['notifications'][0]['reference'] == "some-client-reference" +def test_get_notifications_returns_scheduled_for(client, notify_db, notify_db_session): + sample_notification_with_reference = create_sample_notification( + notify_db, notify_db_session, client_reference='some-client-reference', scheduled_for='2017-05-23 16:00:00') + + auth_header = create_authorization_header(service_id=sample_notification_with_reference.service_id) + response = client.get( + path='/v2/notifications?reference={}'.format(sample_notification_with_reference.client_reference), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 200 + assert response.headers['Content-type'] == 'application/json' + + json_response = json.loads(response.get_data(as_text=True)) + assert len(json_response['notifications']) == 1 + + assert json_response['notifications'][0]['id'] == str(sample_notification_with_reference.id) + assert json_response['notifications'][0]['scheduled_for'] == "2017-05-23T16:00:00.000000Z" + + def test_get_notification_by_reference_nonexistent_reference_returns_no_notifications(client, sample_service): auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( @@ -208,6 +234,7 @@ def test_get_all_notifications_returns_200(client, notify_db, notify_db_session) } assert json_response['notifications'][0]['phone_number'] == "+447700900855" assert json_response['notifications'][0]['type'] == "sms" + assert not json_response['notifications'][0]['scheduled_for'] def test_get_all_notifications_no_notifications_if_no_notifications(client, sample_service): diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 14f8d6d2e..6ead0aeb5 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -1,7 +1,7 @@ import uuid import pytest from flask import json -from app.models import Notification +from app.models import Notification, ScheduledNotification from app.v2.errors import RateLimitError from tests import create_authorization_header from tests.app.conftest import sample_template as create_sample_template, sample_service @@ -41,6 +41,7 @@ def test_post_sms_notification_returns_201(notify_api, sample_template_with_plac assert 'services/{}/templates/{}'.format(sample_template_with_placeholders.service_id, sample_template_with_placeholders.id) \ in resp_json['template']['uri'] + assert not resp_json["scheduled_for"] assert mocked.called @@ -149,6 +150,7 @@ def test_post_email_notification_returns_201(client, sample_email_template_with_ assert 'services/{}/templates/{}'.format(str(sample_email_template_with_placeholders.service_id), str(sample_email_template_with_placeholders.id)) \ in resp_json['template']['uri'] + assert not resp_json["scheduled_for"] assert mocked.called @@ -318,32 +320,56 @@ def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms(notify_db, data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) - print(json.loads(response.get_data(as_text=True))) assert response.status_code == 201 assert response.headers['Content-type'] == 'application/json' -def test_post_sms_should_persist_supplied_sms_number(notify_api, sample_template_with_placeholders, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - data = { - 'phone_number': '+(44) 77009-00855', - 'template_id': str(sample_template_with_placeholders.id), - 'personalisation': {' Name': 'Jo'} - } +def test_post_sms_should_persist_supplied_sms_number(client, sample_template_with_placeholders, mocker): + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = { + 'phone_number': '+(44) 77009-00855', + 'template_id': str(sample_template_with_placeholders.id), + 'personalisation': {' Name': 'Jo'} + } - auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) + auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) - response = client.post( - path='/v2/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 201 - resp_json = json.loads(response.get_data(as_text=True)) - notifications = Notification.query.all() - assert len(notifications) == 1 - notification_id = notifications[0].id - assert '+(44) 77009-00855' == notifications[0].to - assert resp_json['id'] == str(notification_id) - assert mocked.called + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True)) + notifications = Notification.query.all() + assert len(notifications) == 1 + notification_id = notifications[0].id + assert '+(44) 77009-00855' == notifications[0].to + assert resp_json['id'] == str(notification_id) + assert mocked.called + + +@pytest.mark.parametrize("type", ["sms", "email"]) +def test_post_notification_with_scheduled_for(client, sample_template, sample_email_template, type): + if type == 'sms': + data = { + 'phone_number': '+(44) 77009-00855', + 'template_id': str(sample_template.id), + 'scheduled_for': '2017-05-14 15:00:00' + } + else: + data = { + 'email_address': 'jack@blah.com', + 'template_id': str(sample_email_template.id), + 'scheduled_for': '2017-05-14 15:00:00' + } + auth_header = create_authorization_header(service_id=sample_template.service_id) + + response = client.post('/v2/notifications/{}'.format(type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True)) + scheduled_notification = ScheduledNotification.query.all() + assert len(scheduled_notification) == 1 + assert resp_json["id"] == str(scheduled_notification[0].notification_id) + assert resp_json["scheduled_for"] == str(scheduled_notification[0].scheduled_for) From a6529d272306681ed58b17017ab3bf1d2990011f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 16 May 2017 09:57:58 +0100 Subject: [PATCH 05/42] Return the scheduled_for datetime in the DATETIME_FORMAT from the post_notification --- app/v2/notifications/notification_schemas.py | 7 +++-- .../notifications/test_post_notifications.py | 30 +++++++++---------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index c4752ef7b..d75375fcc 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -1,5 +1,6 @@ from datetime import datetime +from app import DATETIME_FORMAT from app.models import NOTIFICATION_STATUS_TYPES, TEMPLATE_TYPES from app.schema_validation.definitions import (uuid, personalisation) @@ -203,7 +204,8 @@ def create_post_sms_response_from_notification(notification, body, from_number, "template": __create_template_from_notification(notification=notification, url_root=url_root, service_id=service_id), - "scheduled_for": scheduled_for + "scheduled_for": datetime.strptime(scheduled_for, + "%Y-%m-%d %H:%M:%S").strftime(DATETIME_FORMAT) if scheduled_for else None } @@ -221,7 +223,8 @@ def create_post_email_response_from_notification(notification, content, subject, "template": __create_template_from_notification(notification=notification, url_root=url_root, service_id=service_id), - "scheduled_for": scheduled_for + "scheduled_for": datetime.strptime(scheduled_for, + "%Y-%m-%d %H:%M:%S").strftime(DATETIME_FORMAT) if scheduled_for else None } diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 6ead0aeb5..4a6abbb63 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -1,6 +1,8 @@ import uuid import pytest from flask import json + +from app import DATETIME_FORMAT from app.models import Notification, ScheduledNotification from app.v2.errors import RateLimitError from tests import create_authorization_header @@ -348,23 +350,19 @@ def test_post_sms_should_persist_supplied_sms_number(client, sample_template_wit assert mocked.called -@pytest.mark.parametrize("type", ["sms", "email"]) -def test_post_notification_with_scheduled_for(client, sample_template, sample_email_template, type): - if type == 'sms': - data = { - 'phone_number': '+(44) 77009-00855', - 'template_id': str(sample_template.id), - 'scheduled_for': '2017-05-14 15:00:00' - } - else: - data = { - 'email_address': 'jack@blah.com', - 'template_id': str(sample_email_template.id), - 'scheduled_for': '2017-05-14 15:00:00' - } +@pytest.mark.parametrize("notification_type, key_send_to, send_to", + [("sms", "phone_number", "07700 900 855"), + ("email", "email_address", "sample@email.com")]) +def test_post_notification_with_scheduled_for(client, sample_template, sample_email_template, + notification_type, key_send_to, send_to): + data = { + key_send_to: send_to, + 'template_id': str(sample_email_template.id) if notification_type == 'email' else str(sample_template.id), + 'scheduled_for': '2017-05-14 15:00:00' + } auth_header = create_authorization_header(service_id=sample_template.service_id) - response = client.post('/v2/notifications/{}'.format(type), + response = client.post('/v2/notifications/{}'.format(notification_type), data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 201 @@ -372,4 +370,4 @@ def test_post_notification_with_scheduled_for(client, sample_template, sample_em scheduled_notification = ScheduledNotification.query.all() assert len(scheduled_notification) == 1 assert resp_json["id"] == str(scheduled_notification[0].notification_id) - assert resp_json["scheduled_for"] == str(scheduled_notification[0].scheduled_for) + assert resp_json["scheduled_for"] == scheduled_notification[0].scheduled_for.strftime(DATETIME_FORMAT) From 579227dfc16474bbce62a45e7282a8c0b16b74a3 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 16 May 2017 10:48:04 +0100 Subject: [PATCH 06/42] Method to return scheduled notifications that are ready to send --- app/dao/notifications_dao.py | 8 +++++++- tests/app/dao/test_notification_dao.py | 12 +++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 1a97418d3..91f8cc774 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -27,7 +27,7 @@ from app.models import ( NOTIFICATION_PERMANENT_FAILURE, KEY_TYPE_NORMAL, KEY_TYPE_TEST, LETTER_TYPE, - NOTIFICATION_SENT) + NOTIFICATION_SENT, ScheduledNotification) from app.dao.dao_utils import transactional from app.statsd_decorators import statsd @@ -469,3 +469,9 @@ def dao_get_notifications_by_to_field(service_id, search_term): def dao_created_scheduled_notification(scheduled_notification): db.session.add(scheduled_notification) db.session.commit() + + +@statsd(namespace="dao") +def dao_get_scheduled_notifications(): + scheduled_notifications = ScheduledNotification.query.filter(ScheduledNotification.scheduled_for < datetime.utcnow()).all() + return scheduled_notifications diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 0f1f3721c..904dc89e1 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -42,7 +42,7 @@ from app.dao.notifications_dao import ( is_delivery_slow_for_provider, dao_update_notifications_sent_to_dvla, dao_get_notifications_by_to_field, - dao_created_scheduled_notification) + dao_created_scheduled_notification, dao_get_scheduled_notifications) from app.dao.services_dao import dao_update_service from tests.app.db import create_notification @@ -1704,3 +1704,13 @@ def test_dao_created_scheduled_notification(sample_notification): assert len(saved_notification) == 1 assert saved_notification[0].notification_id == sample_notification.id assert saved_notification[0].scheduled_for == datetime(2017, 1, 5, 14) + + +def test_dao_get_scheduled_notifications(notify_db, notify_db_session, sample_template): + notification_1 = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_template, scheduled_for='2017-05-05 14:00:00') + sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_template) + scheduled_notifications = dao_get_scheduled_notifications() + assert len(scheduled_notifications) == 1 + assert scheduled_notifications[0].notification_id == notification_1.id From 4aacb3e6ef13ff4c177fc9902d4ebb3905146057 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 16 May 2017 10:51:25 +0100 Subject: [PATCH 07/42] Fix db migration conflict --- migrations/versions/0083_scheduled_notifications.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/migrations/versions/0083_scheduled_notifications.py b/migrations/versions/0083_scheduled_notifications.py index 3e1d295e6..3484b6597 100644 --- a/migrations/versions/0083_scheduled_notifications.py +++ b/migrations/versions/0083_scheduled_notifications.py @@ -1,7 +1,7 @@ """empty message Revision ID: 0083_scheduled_notifications -Revises: 0082_add_go_live_template +Revises: 0083_add_perm_types_and_svc_perm Create Date: 2017-05-15 12:50:20.041950 """ @@ -10,7 +10,7 @@ import sqlalchemy as sa from sqlalchemy.dialects import postgresql revision = '0083_scheduled_notifications' -down_revision = '0082_add_go_live_template' +down_revision = '0083_add_perm_types_and_svc_perm' def upgrade(): From 56f657de9b2d16343716a533b478de3f45a8bb8f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 16 May 2017 11:04:55 +0100 Subject: [PATCH 08/42] fix style --- app/dao/notifications_dao.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 91f8cc774..ab310705f 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -473,5 +473,6 @@ def dao_created_scheduled_notification(scheduled_notification): @statsd(namespace="dao") def dao_get_scheduled_notifications(): - scheduled_notifications = ScheduledNotification.query.filter(ScheduledNotification.scheduled_for < datetime.utcnow()).all() + scheduled_notifications = ScheduledNotification.query.filter( + ScheduledNotification.scheduled_for < datetime.utcnow()).all() return scheduled_notifications From 2e078f9fc8c59042938f65ae4130039c48293632 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 16 May 2017 13:47:22 +0100 Subject: [PATCH 09/42] Add scheduled task to send scheduled notifcations. Fix the query to fetch scheduled notifications. --- app/celery/scheduled_tasks.py | 17 ++++++++++++++-- app/config.py | 5 +++++ app/dao/notifications_dao.py | 10 +++++++--- tests/app/celery/test_scheduled_tasks.py | 25 +++++++++++++++++++++--- tests/app/dao/test_notification_dao.py | 9 ++++++--- 5 files changed, 55 insertions(+), 11 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index b76122819..87eb3ba2f 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -14,13 +14,14 @@ from app.dao.jobs_dao import dao_set_scheduled_jobs_to_pending, dao_get_jobs_old from app.dao.notifications_dao import ( delete_notifications_created_more_than_a_week_ago, dao_timeout_notifications, - is_delivery_slow_for_provider -) + is_delivery_slow_for_provider, + dao_get_scheduled_notifications) from app.dao.provider_details_dao import ( get_current_provider, dao_toggle_sms_provider ) from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago +from app.notifications.process_notifications import send_notification_to_queue from app.statsd_decorators import statsd from app.celery.tasks import process_job @@ -46,6 +47,18 @@ def run_scheduled_jobs(): raise +@notify_celery.task(name='send-scheduled-notifications') +@statsd(namespace="tasks") +def send_scheduled_notifications(): + try: + for notification in dao_get_scheduled_notifications(): + send_notification_to_queue(notification, notification.service.research_mode) + + except SQLAlchemyError as e: + current_app.logger.exception("Failed to send scheduled notifications") + raise + + @notify_celery.task(name="delete-verify-codes") @statsd(namespace="tasks") def delete_verify_codes(): diff --git a/app/config.py b/app/config.py index b87f4d031..807d31071 100644 --- a/app/config.py +++ b/app/config.py @@ -109,6 +109,11 @@ class Config(object): 'schedule': crontab(minute=1), 'options': {'queue': 'periodic'} }, + 'send-scheduled-notifications': { + 'task': 'send-scheduled-notifications', + 'schedule': crontab(minute=1), + 'options': {'queue': 'periodic'} + }, 'delete-verify-codes': { 'task': 'delete-verify-codes', 'schedule': timedelta(minutes=63), diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index ab310705f..2ff6f3a17 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -473,6 +473,10 @@ def dao_created_scheduled_notification(scheduled_notification): @statsd(namespace="dao") def dao_get_scheduled_notifications(): - scheduled_notifications = ScheduledNotification.query.filter( - ScheduledNotification.scheduled_for < datetime.utcnow()).all() - return scheduled_notifications + notifications = Notification.query.join( + ScheduledNotification + ).filter( + ScheduledNotification.scheduled_for < datetime.utcnow(), + Notification.status == NOTIFICATION_CREATED).all() + + return notifications diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 038d2ca1c..f55076d61 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -5,7 +5,7 @@ from functools import partial from flask import current_app from freezegun import freeze_time -from app.celery.scheduled_tasks import s3 +from app.celery.scheduled_tasks import s3, send_scheduled_notifications from app.celery import scheduled_tasks from app.celery.scheduled_tasks import ( delete_verify_codes, @@ -30,8 +30,8 @@ from tests.app.db import create_notification, create_service from tests.app.conftest import ( sample_job as create_sample_job, sample_notification_history as create_notification_history, - create_custom_template -) + create_custom_template, + sample_notification) from tests.conftest import set_config_values from unittest.mock import call, patch, PropertyMock @@ -409,3 +409,22 @@ def test_switch_providers_on_slow_delivery_does_not_switch_based_on_older_notifi switch_current_sms_provider_on_slow_delivery() current_provider = get_current_provider('sms') assert starting_provider.identifier == current_provider.identifier + + +@freeze_time("2017-05-01 14:00:00") +def test_should_send_all_scheduled_notifications_to_deliver_queue(notify_db, + notify_db_session, + sample_template, mocker): + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms') + message_to_deliver = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_template, scheduled_for="2017-05-01 13:50:00") + sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_template, scheduled_for="2017-05-01 10:50:00", status='delivered') + sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_template) + sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_template, scheduled_for="2017-05-01 14:30:00") + + send_scheduled_notifications() + + mocked.apply_async.assert_called_once_with([str(message_to_deliver.id)], queue='send-sms') diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 904dc89e1..f69850f8c 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1708,9 +1708,12 @@ def test_dao_created_scheduled_notification(sample_notification): def test_dao_get_scheduled_notifications(notify_db, notify_db_session, sample_template): notification_1 = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for='2017-05-05 14:00:00') + template=sample_template, scheduled_for='2017-05-05 14:00:00', + status='created') sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template) + template=sample_template, scheduled_for='2017-05-04 14:00:00', status='delivered') + sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_template, status='created') scheduled_notifications = dao_get_scheduled_notifications() assert len(scheduled_notifications) == 1 - assert scheduled_notifications[0].notification_id == notification_1.id + assert scheduled_notifications[0].id == notification_1.id From 10347624894d85c842eea2e460fe8d05bc84321b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 16 May 2017 15:29:31 +0100 Subject: [PATCH 10/42] Ensure the relationship between Notification and ScheduledNotification is one-to-one. Update db script with the right number --- app/models.py | 9 ++++----- ...fications.py => 0084_scheduled_notifications.py} | 5 ++--- tests/app/dao/test_notification_dao.py | 13 +++++++++---- 3 files changed, 15 insertions(+), 12 deletions(-) rename migrations/versions/{0083_scheduled_notifications.py => 0084_scheduled_notifications.py} (87%) diff --git a/app/models.py b/app/models.py index d0f2dfeb7..f52692f5e 100644 --- a/app/models.py +++ b/app/models.py @@ -709,7 +709,7 @@ class Notification(db.Model): foreign(template_version) == remote(TemplateHistory.version) )) - scheduled_for = db.relationship('ScheduledNotification') + scheduled_notification = db.relationship('ScheduledNotification', uselist=False) client_reference = db.Column(db.String, index=True, nullable=True) @@ -872,8 +872,8 @@ class Notification(db.Model): "created_at": self.created_at.strftime(DATETIME_FORMAT), "sent_at": self.sent_at.strftime(DATETIME_FORMAT) if self.sent_at else None, "completed_at": self.completed_at(), - "scheduled_for": self.scheduled_for[0].scheduled_for.strftime( - DATETIME_FORMAT) if self.scheduled_for else None + "scheduled_for": self.scheduled_notification.scheduled_for.strftime( + DATETIME_FORMAT) if self.scheduled_notification else None } return serialized @@ -944,9 +944,8 @@ class ScheduledNotification(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4()) notification_id = db.Column(UUID(as_uuid=True), db.ForeignKey('notifications.id'), index=True, nullable=False) - notification = db.relationship('Notification') + notification = db.relationship('Notification', uselist=False) scheduled_for = db.Column(db.DateTime, index=False, nullable=False) - pending = db.Column(db.Boolean, nullable=False, default=False) class InvitedUser(db.Model): diff --git a/migrations/versions/0083_scheduled_notifications.py b/migrations/versions/0084_scheduled_notifications.py similarity index 87% rename from migrations/versions/0083_scheduled_notifications.py rename to migrations/versions/0084_scheduled_notifications.py index 3484b6597..6405ea2a9 100644 --- a/migrations/versions/0083_scheduled_notifications.py +++ b/migrations/versions/0084_scheduled_notifications.py @@ -1,6 +1,6 @@ """empty message -Revision ID: 0083_scheduled_notifications +Revision ID: 0084_scheduled_notifications Revises: 0083_add_perm_types_and_svc_perm Create Date: 2017-05-15 12:50:20.041950 @@ -9,7 +9,7 @@ from alembic import op import sqlalchemy as sa from sqlalchemy.dialects import postgresql -revision = '0083_scheduled_notifications' +revision = '0084_scheduled_notifications' down_revision = '0083_add_perm_types_and_svc_perm' @@ -18,7 +18,6 @@ def upgrade(): sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), sa.Column('notification_id', postgresql.UUID(as_uuid=True), nullable=False), sa.Column('scheduled_for', sa.DateTime(), nullable=False), - sa.Column('pending', sa.Boolean(), nullable=False), sa.ForeignKeyConstraint(['notification_id'], ['notifications.id'], ), sa.PrimaryKeyConstraint('id') ) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index f69850f8c..832c80949 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -716,13 +716,18 @@ def test_save_notification_with_no_job(sample_template, mmg_provider): assert notification_from_db.status == 'created' -def test_get_notification_by_id(sample_notification): +def test_get_notification_by_id(notify_db, notify_db_session, client, sample_template): + notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_template, + scheduled_for='2017-05-05 14:00:00', + status='created') notification_from_db = get_notification_with_personalisation( - sample_notification.service.id, - sample_notification.id, + sample_template.service.id, + notification.id, key_type=None ) - assert sample_notification == notification_from_db + assert notification == notification_from_db + assert notification_from_db.scheduled_notification.scheduled_for == datetime(2017, 5, 5, 14, 0) def test_get_notifications_by_reference(notify_db, notify_db_session, sample_service): From 5f8338dd805c52787a060dc818e975f86d81b0c4 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 17 May 2017 12:59:00 +0100 Subject: [PATCH 11/42] Remove the beat job. Add logging to the task. --- app/celery/scheduled_tasks.py | 6 ++++-- app/config.py | 5 ----- celerybeat.pid | 1 - 3 files changed, 4 insertions(+), 8 deletions(-) delete mode 100644 celerybeat.pid diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 87eb3ba2f..27b2b8953 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -51,9 +51,11 @@ def run_scheduled_jobs(): @statsd(namespace="tasks") def send_scheduled_notifications(): try: - for notification in dao_get_scheduled_notifications(): + scheduled_notifications = dao_get_scheduled_notifications() + for notification in scheduled_notifications: send_notification_to_queue(notification, notification.service.research_mode) - + current_app.logger.info( + "Sent {} scheudled notifications to the provider queue".format(len(scheduled_notifications))) except SQLAlchemyError as e: current_app.logger.exception("Failed to send scheduled notifications") raise diff --git a/app/config.py b/app/config.py index 807d31071..b87f4d031 100644 --- a/app/config.py +++ b/app/config.py @@ -109,11 +109,6 @@ class Config(object): 'schedule': crontab(minute=1), 'options': {'queue': 'periodic'} }, - 'send-scheduled-notifications': { - 'task': 'send-scheduled-notifications', - 'schedule': crontab(minute=1), - 'options': {'queue': 'periodic'} - }, 'delete-verify-codes': { 'task': 'delete-verify-codes', 'schedule': timedelta(minutes=63), diff --git a/celerybeat.pid b/celerybeat.pid deleted file mode 100644 index d2acb3a18..000000000 --- a/celerybeat.pid +++ /dev/null @@ -1 +0,0 @@ -9772 From 973cc2c4c96be9954a5200af77b373a304ec8cb4 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 17 May 2017 15:06:15 +0100 Subject: [PATCH 12/42] Changed the scheduled_for datetime to only send and hour of a day to send. Also expect the date being passed in is BST. The date is converted to UTC before saving. And converted to BST when returning a notification. --- .../performance_platform_client.py | 6 +++--- app/models.py | 6 +++--- app/notifications/process_notifications.py | 5 +++-- app/schema_validation/__init__.py | 6 +++--- app/utils.py | 6 +++++- app/v2/notifications/notification_schemas.py | 11 +++-------- tests/app/celery/test_scheduled_tasks.py | 6 +++--- tests/app/conftest.py | 2 +- tests/app/dao/test_notification_dao.py | 13 +++++++------ .../app/notifications/test_process_notification.py | 13 +++++++++++-- tests/app/test_utils.py | 14 +++++++++++--- .../app/v2/notifications/test_get_notifications.py | 10 +++++----- .../v2/notifications/test_notification_schemas.py | 6 +++--- .../v2/notifications/test_post_notifications.py | 6 +++--- 14 files changed, 64 insertions(+), 46 deletions(-) diff --git a/app/clients/performance_platform/performance_platform_client.py b/app/clients/performance_platform/performance_platform_client.py index 91e43a210..d8685f0f3 100644 --- a/app/clients/performance_platform/performance_platform_client.py +++ b/app/clients/performance_platform/performance_platform_client.py @@ -1,11 +1,11 @@ import base64 import json -from datetime import datetime, timedelta +from datetime import datetime import requests from flask import current_app -from app.utils import get_midnight_for_day_before, get_london_midnight_in_utc, get_utc_time_in_bst +from app.utils import get_midnight_for_day_before, get_london_midnight_in_utc, convert_utc_time_in_bst class PerformancePlatformClient: @@ -27,7 +27,7 @@ class PerformancePlatformClient: def send_performance_stats(self, date, channel, count, period): if self.active: payload = { - '_timestamp': get_utc_time_in_bst(date).isoformat(), + '_timestamp': convert_utc_time_in_bst(date).isoformat(), 'service': 'govuk-notify', 'channel': channel, 'count': count, diff --git a/app/models.py b/app/models.py index f52692f5e..54cc098b2 100644 --- a/app/models.py +++ b/app/models.py @@ -29,7 +29,7 @@ from app import ( ) from app.history_meta import Versioned -from app.utils import get_utc_time_in_bst +from app.utils import convert_utc_time_in_bst SMS_TYPE = 'sms' EMAIL_TYPE = 'email' @@ -832,7 +832,7 @@ class Notification(db.Model): }[self.template.template_type].get(self.status, self.status) def serialize_for_csv(self): - created_at_in_bst = get_utc_time_in_bst(self.created_at) + created_at_in_bst = convert_utc_time_in_bst(self.created_at) serialized = { "row_number": '' if self.job_row_number is None else self.job_row_number + 1, "recipient": self.to, @@ -873,7 +873,7 @@ class Notification(db.Model): "sent_at": self.sent_at.strftime(DATETIME_FORMAT) if self.sent_at else None, "completed_at": self.completed_at(), "scheduled_for": self.scheduled_notification.scheduled_for.strftime( - DATETIME_FORMAT) if self.scheduled_notification else None + "%Y-%m-%d %H") if self.scheduled_notification else None } return serialized diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index bb88f1265..bfa16d306 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -15,7 +15,7 @@ from app.dao.notifications_dao import (dao_create_notification, dao_created_scheduled_notification) from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE, ScheduledNotification from app.v2.errors import BadRequestError, SendNotificationToQueueError -from app.utils import get_template_instance, cache_key_for_service_template_counter +from app.utils import get_template_instance, cache_key_for_service_template_counter, convert_bst_to_utc def create_content_for_notification(template, personalisation): @@ -125,6 +125,7 @@ def simulated_recipient(to_address, notification_type): def persist_scheduled_notification(notification_id, scheduled_for): + scheduled_datetime = convert_bst_to_utc(datetime.strptime(scheduled_for, "%Y-%m-%d %H")) scheduled_notification = ScheduledNotification(notification_id=notification_id, - scheduled_for=scheduled_for) + scheduled_for=scheduled_datetime) dao_created_scheduled_notification(scheduled_notification) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index fc134e2ce..4d9d0bc55 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -22,13 +22,13 @@ def validate(json_to_validate, schema): return True @format_checker.checks('datetime', raises=ValidationError) - def validate_schema_datetime(instance): + def validate_schema_date_with_hour(instance): if isinstance(instance, str): try: - datetime.strptime(instance, "%Y-%m-%d %H:%M:%S") + datetime.strptime(instance, "%Y-%m-%d %H") except ValueError as e: raise ValidationError("datetime format is invalid. Use the format: " - "YYYY-MM-DD HH:MM:SS, for example 2017-05-30 13:00:00") + "YYYY-MM-DD HH, for example 2017-05-30 13") return True validator = Draft4Validator(schema, format_checker=format_checker) diff --git a/app/utils.py b/app/utils.py index 34bb1ec3a..d93ea0613 100644 --- a/app/utils.py +++ b/app/utils.py @@ -51,10 +51,14 @@ def get_midnight_for_day_before(date): return get_london_midnight_in_utc(day_before) -def get_utc_time_in_bst(utc_dt): +def convert_utc_time_in_bst(utc_dt): return pytz.utc.localize(utc_dt).astimezone(local_timezone).replace(tzinfo=None) +def convert_bst_to_utc(date): + return local_timezone.localize(date).astimezone(pytz.UTC).replace(tzinfo=None) + + def get_london_month_from_utc_column(column): """ Where queries need to count notifications by month it needs to be diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index d75375fcc..6aa4dcb77 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -1,10 +1,7 @@ -from datetime import datetime - -from app import DATETIME_FORMAT from app.models import NOTIFICATION_STATUS_TYPES, TEMPLATE_TYPES from app.schema_validation.definitions import (uuid, personalisation) -# this may belong in a templates module + template = { "$schema": "http://json-schema.org/draft-04/schema#", "description": "template schema", @@ -204,8 +201,7 @@ def create_post_sms_response_from_notification(notification, body, from_number, "template": __create_template_from_notification(notification=notification, url_root=url_root, service_id=service_id), - "scheduled_for": datetime.strptime(scheduled_for, - "%Y-%m-%d %H:%M:%S").strftime(DATETIME_FORMAT) if scheduled_for else None + "scheduled_for": scheduled_for if scheduled_for else None } @@ -223,8 +219,7 @@ def create_post_email_response_from_notification(notification, content, subject, "template": __create_template_from_notification(notification=notification, url_root=url_root, service_id=service_id), - "scheduled_for": datetime.strptime(scheduled_for, - "%Y-%m-%d %H:%M:%S").strftime(DATETIME_FORMAT) if scheduled_for else None + "scheduled_for": scheduled_for if scheduled_for else None } diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index f55076d61..a2564e1cd 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -417,13 +417,13 @@ def test_should_send_all_scheduled_notifications_to_deliver_queue(notify_db, sample_template, mocker): mocked = mocker.patch('app.celery.provider_tasks.deliver_sms') message_to_deliver = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for="2017-05-01 13:50:00") + template=sample_template, scheduled_for="2017-05-01 13") sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for="2017-05-01 10:50:00", status='delivered') + template=sample_template, scheduled_for="2017-05-01 10", status='delivered') sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, template=sample_template) sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for="2017-05-01 14:30:00") + template=sample_template, scheduled_for="2017-05-01 14") send_scheduled_notifications() diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 7e859324f..26e3e4dc0 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -494,7 +494,7 @@ def sample_notification( scheduled_notification = ScheduledNotification(id=uuid.uuid4(), notification_id=notification.id, scheduled_for=datetime.strptime(scheduled_for, - "%Y-%m-%d %H:%M:%S")) + "%Y-%m-%d %H")) db.session.add(scheduled_notification) db.session.commit() diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 832c80949..34e27c15b 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -716,10 +716,10 @@ def test_save_notification_with_no_job(sample_template, mmg_provider): assert notification_from_db.status == 'created' -def test_get_notification_by_id(notify_db, notify_db_session, client, sample_template): +def test_get_notification_by_id(notify_db, notify_db_session, sample_template): notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, template=sample_template, - scheduled_for='2017-05-05 14:00:00', + scheduled_for='2017-05-05 14', status='created') notification_from_db = get_notification_with_personalisation( sample_template.service.id, @@ -727,7 +727,7 @@ def test_get_notification_by_id(notify_db, notify_db_session, client, sample_tem key_type=None ) assert notification == notification_from_db - assert notification_from_db.scheduled_notification.scheduled_for == datetime(2017, 5, 5, 14, 0) + assert notification_from_db.scheduled_notification.scheduled_for == datetime(2017, 5, 5, 14) def test_get_notifications_by_reference(notify_db, notify_db_session, sample_service): @@ -1702,8 +1702,9 @@ def test_dao_get_notifications_by_to_field_search_ignores_spaces(sample_template def test_dao_created_scheduled_notification(sample_notification): + scheduled_notification = ScheduledNotification(notification_id=sample_notification.id, - scheduled_for="2017-01-05 14:00:00") + scheduled_for=datetime.strptime("2017-01-05 14", "%Y-%m-%d %H")) dao_created_scheduled_notification(scheduled_notification) saved_notification = ScheduledNotification.query.all() assert len(saved_notification) == 1 @@ -1713,10 +1714,10 @@ def test_dao_created_scheduled_notification(sample_notification): def test_dao_get_scheduled_notifications(notify_db, notify_db_session, sample_template): notification_1 = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for='2017-05-05 14:00:00', + template=sample_template, scheduled_for='2017-05-05 14', status='created') sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for='2017-05-04 14:00:00', status='delivered') + template=sample_template, scheduled_for='2017-05-04 14', status='delivered') sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, template=sample_template, status='created') scheduled_notifications = dao_get_scheduled_notifications() diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index f697047e7..dc0e64b57 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -7,13 +7,14 @@ from sqlalchemy.exc import SQLAlchemyError from freezegun import freeze_time from collections import namedtuple -from app.models import Template, Notification, NotificationHistory +from app.models import Template, Notification, NotificationHistory, ScheduledNotification from app.notifications import SendNotificationToQueueError from app.notifications.process_notifications import ( create_content_for_notification, persist_notification, send_notification_to_queue, - simulated_recipient + simulated_recipient, + persist_scheduled_notification ) from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address from app.utils import cache_key_for_service_template_counter @@ -358,3 +359,11 @@ def test_persist_notification_with_international_info_does_not_store_for_email( assert persisted_notification.international is False assert persisted_notification.phone_prefix is None assert persisted_notification.rate_multiplier is None + + +def test_persist_scheduled_notification(sample_notification): + persist_scheduled_notification(sample_notification.id, '2017-05-12 14') + scheduled_notification = ScheduledNotification.query.all() + assert len(scheduled_notification) == 1 + assert scheduled_notification[0].notification_id == sample_notification.id + assert scheduled_notification[0].scheduled_for == datetime.datetime(2017, 5, 12, 13, 0) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 5e45bdf7f..7bb8360af 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -4,8 +4,8 @@ import pytest from app.utils import ( get_london_midnight_in_utc, get_midnight_for_day_before, - get_utc_time_in_bst -) + convert_utc_time_in_bst, + convert_bst_to_utc) @pytest.mark.parametrize('date, expected_date', [ @@ -32,7 +32,15 @@ def test_get_midnight_for_day_before_returns_expected_date(date, expected_date): (datetime(2017, 3, 28, 10, 0), datetime(2017, 3, 28, 11, 0)), (datetime(2017, 10, 28, 1, 0), datetime(2017, 10, 28, 2, 0)), (datetime(2017, 10, 29, 1, 0), datetime(2017, 10, 29, 1, 0)), + (datetime(2017, 5, 12, 14), datetime(2017, 5, 12, 15, 0)) ]) def test_get_utc_in_bst_returns_expected_date(date, expected_date): - ret_date = get_utc_time_in_bst(date) + ret_date = convert_utc_time_in_bst(date) assert ret_date == expected_date + + +def test_convert_bst_to_utc(): + bst = "2017-05-12 13" + bst_datetime = datetime.strptime(bst, "%Y-%m-%d %H") + utc = convert_bst_to_utc(bst_datetime) + assert utc == datetime(2017, 5, 12, 12, 0) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 2b8fd0832..30a35b5aa 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -20,12 +20,12 @@ def test_get_notification_by_id_returns_200( ): sample_notification = create_sample_notification( notify_db, notify_db_session, billable_units=billable_units, sent_by=provider, - scheduled_for="2017-05-12 14:00:00" + scheduled_for="2017-05-12 14" ) another = create_sample_notification( notify_db, notify_db_session, billable_units=billable_units, sent_by=provider, - scheduled_for="2017-06-12 14:00:00" + scheduled_for="2017-06-12 14" ) auth_header = create_authorization_header(service_id=sample_notification.service_id) response = client.get( @@ -63,7 +63,7 @@ def test_get_notification_by_id_returns_200( "subject": None, 'sent_at': sample_notification.sent_at, 'completed_at': sample_notification.completed_at(), - 'scheduled_for': '2017-05-12T14:00:00.000000Z' + 'scheduled_for': '2017-05-12 14' } assert json_response == expected_response @@ -139,7 +139,7 @@ def test_get_notification_by_reference_returns_200(client, notify_db, notify_db_ def test_get_notifications_returns_scheduled_for(client, notify_db, notify_db_session): sample_notification_with_reference = create_sample_notification( - notify_db, notify_db_session, client_reference='some-client-reference', scheduled_for='2017-05-23 16:00:00') + notify_db, notify_db_session, client_reference='some-client-reference', scheduled_for='2017-05-23 16') auth_header = create_authorization_header(service_id=sample_notification_with_reference.service_id) response = client.get( @@ -153,7 +153,7 @@ def test_get_notifications_returns_scheduled_for(client, notify_db, notify_db_se assert len(json_response['notifications']) == 1 assert json_response['notifications'][0]['id'] == str(sample_notification_with_reference.id) - assert json_response['notifications'][0]['scheduled_for'] == "2017-05-23T16:00:00.000000Z" + assert json_response['notifications'][0]['scheduled_for'] == "2017-05-23 16" def test_get_notification_by_reference_nonexistent_reference_returns_no_notifications(client, sample_service): diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 44794f9de..5e0cb354f 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -360,7 +360,7 @@ def test_get_notifications_response_with_email_and_phone_number(): def test_post_schema_valid_scheduled_for(schema): j = {"template_id": str(uuid.uuid4()), "email_address": "joe@gmail.com", - "scheduled_for": "2017-05-12 13:00:00"} + "scheduled_for": "2017-05-12 13"} if schema == post_email_request_schema: j.update({"email_address": "joe@gmail.com"}) else: @@ -369,7 +369,7 @@ def test_post_schema_valid_scheduled_for(schema): @pytest.mark.parametrize("invalid_datetime", - ["2017-05-12 13:00", "13:00:00 2017-01-01"]) + ["2017-05-12 13:00:00", "13:00:00 2017-01-01"]) @pytest.mark.parametrize("schema", [post_email_request_schema, post_sms_request_schema]) def test_post_email_schema_invalid_scheduled_for(invalid_datetime, schema): @@ -385,4 +385,4 @@ def test_post_email_schema_invalid_scheduled_for(invalid_datetime, schema): assert error['status_code'] == 400 assert error['errors'] == [{'error': 'ValidationError', 'message': "scheduled_for datetime format is invalid. Use the format: " - "YYYY-MM-DD HH:MM:SS, for example 2017-05-30 13:00:00"}] + "YYYY-MM-DD HH, for example 2017-05-30 13"}] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 4a6abbb63..e0903ef6b 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -1,8 +1,8 @@ import uuid + import pytest from flask import json -from app import DATETIME_FORMAT from app.models import Notification, ScheduledNotification from app.v2.errors import RateLimitError from tests import create_authorization_header @@ -358,7 +358,7 @@ def test_post_notification_with_scheduled_for(client, sample_template, sample_em data = { key_send_to: send_to, 'template_id': str(sample_email_template.id) if notification_type == 'email' else str(sample_template.id), - 'scheduled_for': '2017-05-14 15:00:00' + 'scheduled_for': '2017-05-14 14' } auth_header = create_authorization_header(service_id=sample_template.service_id) @@ -370,4 +370,4 @@ def test_post_notification_with_scheduled_for(client, sample_template, sample_em scheduled_notification = ScheduledNotification.query.all() assert len(scheduled_notification) == 1 assert resp_json["id"] == str(scheduled_notification[0].notification_id) - assert resp_json["scheduled_for"] == scheduled_notification[0].scheduled_for.strftime(DATETIME_FORMAT) + assert resp_json["scheduled_for"] == '2017-05-14 14' From 751abb4b999dfd4ebab4bda3dda4d5675a2eb048 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 22 May 2017 14:15:35 +0100 Subject: [PATCH 13/42] Update dateformat for scheduled_for to include minutes. --- app/notifications/process_notifications.py | 2 +- app/schema_validation/__init__.py | 4 ++-- tests/app/notifications/test_process_notification.py | 4 ++-- tests/app/v2/notifications/test_notification_schemas.py | 4 ++-- tests/app/v2/notifications/test_post_notifications.py | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 4383858bb..20550be7a 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -128,7 +128,7 @@ def simulated_recipient(to_address, notification_type): def persist_scheduled_notification(notification_id, scheduled_for): - scheduled_datetime = convert_bst_to_utc(datetime.strptime(scheduled_for, "%Y-%m-%d %H")) + scheduled_datetime = convert_bst_to_utc(datetime.strptime(scheduled_for, "%Y-%m-%d %H:%M")) scheduled_notification = ScheduledNotification(notification_id=notification_id, scheduled_for=scheduled_datetime) dao_created_scheduled_notification(scheduled_notification) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 4d9d0bc55..b530a17b0 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -25,10 +25,10 @@ def validate(json_to_validate, schema): def validate_schema_date_with_hour(instance): if isinstance(instance, str): try: - datetime.strptime(instance, "%Y-%m-%d %H") + datetime.strptime(instance, "%Y-%m-%d %H:%M") except ValueError as e: raise ValidationError("datetime format is invalid. Use the format: " - "YYYY-MM-DD HH, for example 2017-05-30 13") + "YYYY-MM-DD HH:MI, for example 2017-05-30 13:15") return True validator = Draft4Validator(schema, format_checker=format_checker) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 56d356842..6df3862a3 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -343,8 +343,8 @@ def test_persist_notification_with_international_info_does_not_store_for_email( def test_persist_scheduled_notification(sample_notification): - persist_scheduled_notification(sample_notification.id, '2017-05-12 14') + persist_scheduled_notification(sample_notification.id, '2017-05-12 14:15') scheduled_notification = ScheduledNotification.query.all() assert len(scheduled_notification) == 1 assert scheduled_notification[0].notification_id == sample_notification.id - assert scheduled_notification[0].scheduled_for == datetime.datetime(2017, 5, 12, 13, 0) + assert scheduled_notification[0].scheduled_for == datetime.datetime(2017, 5, 12, 13, 15) diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 5e0cb354f..90139c7c2 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -360,7 +360,7 @@ def test_get_notifications_response_with_email_and_phone_number(): def test_post_schema_valid_scheduled_for(schema): j = {"template_id": str(uuid.uuid4()), "email_address": "joe@gmail.com", - "scheduled_for": "2017-05-12 13"} + "scheduled_for": "2017-05-12 13:15"} if schema == post_email_request_schema: j.update({"email_address": "joe@gmail.com"}) else: @@ -385,4 +385,4 @@ def test_post_email_schema_invalid_scheduled_for(invalid_datetime, schema): assert error['status_code'] == 400 assert error['errors'] == [{'error': 'ValidationError', 'message': "scheduled_for datetime format is invalid. Use the format: " - "YYYY-MM-DD HH, for example 2017-05-30 13"}] + "YYYY-MM-DD HH:MI, for example 2017-05-30 13:15"}] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index e0903ef6b..114e2e8aa 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -358,7 +358,7 @@ def test_post_notification_with_scheduled_for(client, sample_template, sample_em data = { key_send_to: send_to, 'template_id': str(sample_email_template.id) if notification_type == 'email' else str(sample_template.id), - 'scheduled_for': '2017-05-14 14' + 'scheduled_for': '2017-05-14 14:15' } auth_header = create_authorization_header(service_id=sample_template.service_id) @@ -370,4 +370,4 @@ def test_post_notification_with_scheduled_for(client, sample_template, sample_em scheduled_notification = ScheduledNotification.query.all() assert len(scheduled_notification) == 1 assert resp_json["id"] == str(scheduled_notification[0].notification_id) - assert resp_json["scheduled_for"] == '2017-05-14 14' + assert resp_json["scheduled_for"] == '2017-05-14 14:15' From a57dc188952b1f164d7aecfb7c98a8965227e450 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 22 May 2017 14:39:30 +0100 Subject: [PATCH 14/42] Add validation for scheduled_for where the date can not be in the past or more than 24 hours in the future. --- app/schema_validation/__init__.py | 8 ++++-- .../test_notification_schemas.py | 28 +++++++++++++++++++ .../notifications/test_post_notifications.py | 2 ++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index b530a17b0..2dff49519 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -1,5 +1,5 @@ import json -from datetime import datetime +from datetime import datetime, timedelta from jsonschema import (Draft4Validator, ValidationError, FormatChecker) from notifications_utils.recipients import (validate_phone_number, validate_email_address, InvalidPhoneError, @@ -25,7 +25,11 @@ def validate(json_to_validate, schema): def validate_schema_date_with_hour(instance): if isinstance(instance, str): try: - datetime.strptime(instance, "%Y-%m-%d %H:%M") + dt = datetime.strptime(instance, "%Y-%m-%d %H:%M") + if dt < datetime.utcnow(): + raise ValidationError("datetime can not be in the past") + if dt > datetime.utcnow() + timedelta(hours=24): + raise ValidationError("datetime can only be 24 hours in the future") except ValueError as e: raise ValidationError("datetime format is invalid. Use the format: " "YYYY-MM-DD HH:MI, for example 2017-05-30 13:15") diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 90139c7c2..bbccd81d5 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -2,6 +2,7 @@ import uuid import pytest from flask import json +from freezegun import freeze_time from jsonschema import ValidationError from app.v2.notifications.notification_schemas import ( @@ -357,6 +358,7 @@ def test_get_notifications_response_with_email_and_phone_number(): @pytest.mark.parametrize("schema", [post_email_request_schema, post_sms_request_schema]) +@freeze_time("2017-05-12 13:00:00") def test_post_schema_valid_scheduled_for(schema): j = {"template_id": str(uuid.uuid4()), "email_address": "joe@gmail.com", @@ -386,3 +388,29 @@ def test_post_email_schema_invalid_scheduled_for(invalid_datetime, schema): assert error['errors'] == [{'error': 'ValidationError', 'message': "scheduled_for datetime format is invalid. Use the format: " "YYYY-MM-DD HH:MI, for example 2017-05-30 13:15"}] + + +@freeze_time("2017-05-12 13:00:00") +def test_scheduled_for_raises_validation_error_when_in_the_past(): + j = {"phone_number": "07515111111", + "template_id": str(uuid.uuid4()), + "scheduled_for": "2017-05-12 10:00"} + with pytest.raises(ValidationError) as e: + validate(j, post_sms_request_schema) + error = json.loads(str(e.value)) + assert error['status_code'] == 400 + assert error['errors'] == [{'error': 'ValidationError', + 'message': "scheduled_for datetime can not be in the past"}] + + +@freeze_time("2017-05-12 13:00:00") +def test_scheduled_for_raises_validation_error_when_more_than_24_hours_in_the_future(): + j = {"phone_number": "07515111111", + "template_id": str(uuid.uuid4()), + "scheduled_for": "2017-05-13 14:00"} + with pytest.raises(ValidationError) as e: + validate(j, post_sms_request_schema) + error = json.loads(str(e.value)) + assert error['status_code'] == 400 + assert error['errors'] == [{'error': 'ValidationError', + 'message': "scheduled_for datetime can only be 24 hours in the future"}] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 114e2e8aa..305d224b8 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -2,6 +2,7 @@ import uuid import pytest from flask import json +from freezegun import freeze_time from app.models import Notification, ScheduledNotification from app.v2.errors import RateLimitError @@ -353,6 +354,7 @@ def test_post_sms_should_persist_supplied_sms_number(client, sample_template_wit @pytest.mark.parametrize("notification_type, key_send_to, send_to", [("sms", "phone_number", "07700 900 855"), ("email", "email_address", "sample@email.com")]) +@freeze_time("2017-05-14 14:00:00") def test_post_notification_with_scheduled_for(client, sample_template, sample_email_template, notification_type, key_send_to, send_to): data = { From 9bfba52f533eb2583c5f4944d2707e579d3825ce Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 22 May 2017 15:07:16 +0100 Subject: [PATCH 15/42] Add pending flag to scheduled_notifications. Set pending flag to false when the notification has been sent to provider task. --- app/celery/scheduled_tasks.py | 5 +++-- app/dao/notifications_dao.py | 10 +++++++++- app/models.py | 1 + .../versions/0085_scheduled_notifications.py | 1 + tests/app/celery/test_scheduled_tasks.py | 6 ++++++ tests/app/conftest.py | 2 ++ tests/app/dao/test_notification_dao.py | 17 ++++++++++++++++- 7 files changed, 38 insertions(+), 4 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index d8d5567f6..56b3734a5 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -15,8 +15,8 @@ from app.dao.notifications_dao import ( delete_notifications_created_more_than_a_week_ago, dao_timeout_notifications, is_delivery_slow_for_provider, - dao_get_scheduled_notifications -) + dao_get_scheduled_notifications, + set_scheduled_notification_to_processed) from app.dao.statistics_dao import dao_timeout_job_statistics from app.dao.provider_details_dao import ( get_current_provider, @@ -56,6 +56,7 @@ def send_scheduled_notifications(): scheduled_notifications = dao_get_scheduled_notifications() for notification in scheduled_notifications: send_notification_to_queue(notification, notification.service.research_mode) + set_scheduled_notification_to_processed(notification.id) current_app.logger.info( "Sent {} scheudled notifications to the provider queue".format(len(scheduled_notifications))) except SQLAlchemyError as e: diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 2ff6f3a17..75acf5152 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -477,6 +477,14 @@ def dao_get_scheduled_notifications(): ScheduledNotification ).filter( ScheduledNotification.scheduled_for < datetime.utcnow(), - Notification.status == NOTIFICATION_CREATED).all() + ScheduledNotification.pending).all() return notifications + + +def set_scheduled_notification_to_processed(notification_id): + ScheduledNotification.query.filter( + ScheduledNotification.notification_id == notification_id + ).update( + {'pending': False} + ) diff --git a/app/models.py b/app/models.py index cdbd7ba69..915a2fff2 100644 --- a/app/models.py +++ b/app/models.py @@ -958,6 +958,7 @@ class ScheduledNotification(db.Model): notification_id = db.Column(UUID(as_uuid=True), db.ForeignKey('notifications.id'), index=True, nullable=False) notification = db.relationship('Notification', uselist=False) scheduled_for = db.Column(db.DateTime, index=False, nullable=False) + pending = db.Column(db.Boolean, nullable=False, default=True) class InvitedUser(db.Model): diff --git a/migrations/versions/0085_scheduled_notifications.py b/migrations/versions/0085_scheduled_notifications.py index a415d69ee..cd0c932a9 100644 --- a/migrations/versions/0085_scheduled_notifications.py +++ b/migrations/versions/0085_scheduled_notifications.py @@ -18,6 +18,7 @@ def upgrade(): sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), sa.Column('notification_id', postgresql.UUID(as_uuid=True), nullable=False), sa.Column('scheduled_for', sa.DateTime(), nullable=False), + sa.Column('pending', sa.Boolean, nullable=False, default=True), sa.ForeignKeyConstraint(['notification_id'], ['notifications.id'], ), sa.PrimaryKeyConstraint('id') ) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 465deb101..97c184cdf 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -20,6 +20,7 @@ from app.celery.scheduled_tasks import ( ) from app.clients.performance_platform.performance_platform_client import PerformancePlatformClient from app.dao.jobs_dao import dao_get_job_by_id +from app.dao.notifications_dao import dao_get_scheduled_notifications from app.dao.provider_details_dao import ( dao_update_provider_details, get_current_provider @@ -425,9 +426,14 @@ def test_should_send_all_scheduled_notifications_to_deliver_queue(notify_db, sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, template=sample_template, scheduled_for="2017-05-01 14") + scheduled_notifications = dao_get_scheduled_notifications() + assert len(scheduled_notifications) == 1 + send_scheduled_notifications() mocked.apply_async.assert_called_once_with([str(message_to_deliver.id)], queue='send-sms') + scheduled_notifications = dao_get_scheduled_notifications() + assert not scheduled_notifications def test_timeout_job_statistics_called_with_notification_timeout(notify_api, mocker): diff --git a/tests/app/conftest.py b/tests/app/conftest.py index b1734e10f..032a134fc 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -495,6 +495,8 @@ def sample_notification( notification_id=notification.id, scheduled_for=datetime.strptime(scheduled_for, "%Y-%m-%d %H")) + if status != 'created': + scheduled_notification.pending = False db.session.add(scheduled_notification) db.session.commit() diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 34e27c15b..c467cf845 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -42,7 +42,7 @@ from app.dao.notifications_dao import ( is_delivery_slow_for_provider, dao_update_notifications_sent_to_dvla, dao_get_notifications_by_to_field, - dao_created_scheduled_notification, dao_get_scheduled_notifications) + dao_created_scheduled_notification, dao_get_scheduled_notifications, set_scheduled_notification_to_processed) from app.dao.services_dao import dao_update_service from tests.app.db import create_notification @@ -1723,3 +1723,18 @@ def test_dao_get_scheduled_notifications(notify_db, notify_db_session, sample_te scheduled_notifications = dao_get_scheduled_notifications() assert len(scheduled_notifications) == 1 assert scheduled_notifications[0].id == notification_1.id + assert scheduled_notifications[0].scheduled_notification.pending + + +def test_set_scheduled_notification_to_processed(notify_db, notify_db_session, sample_template): + notification_1 = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, + template=sample_template, scheduled_for='2017-05-05 14', + status='created') + scheduled_notifications = dao_get_scheduled_notifications() + assert len(scheduled_notifications) == 1 + assert scheduled_notifications[0].id == notification_1.id + assert scheduled_notifications[0].scheduled_notification.pending + + set_scheduled_notification_to_processed(notification_1.id) + scheduled_notifications = dao_get_scheduled_notifications() + assert not scheduled_notifications From 76d0783c63a8c8440ea4d63356924e0954fd73a3 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 22 May 2017 16:30:45 +0100 Subject: [PATCH 16/42] Add beat config for send-scheduled-notifications task to run every 15 minutes. Added the missing commit to the update pending --- app/celery/scheduled_tasks.py | 6 +++--- app/config.py | 5 +++++ app/dao/notifications_dao.py | 3 ++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 56b3734a5..c20f8ec7e 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -44,7 +44,7 @@ def run_scheduled_jobs(): for job in dao_set_scheduled_jobs_to_pending(): process_job.apply_async([str(job.id)], queue="process-job") current_app.logger.info("Job ID {} added to process job queue".format(job.id)) - except SQLAlchemyError as e: + except SQLAlchemyError: current_app.logger.exception("Failed to run scheduled jobs") raise @@ -58,8 +58,8 @@ def send_scheduled_notifications(): send_notification_to_queue(notification, notification.service.research_mode) set_scheduled_notification_to_processed(notification.id) current_app.logger.info( - "Sent {} scheudled notifications to the provider queue".format(len(scheduled_notifications))) - except SQLAlchemyError as e: + "Sent {} scheduled notifications to the provider queue".format(len(scheduled_notifications))) + except SQLAlchemyError: current_app.logger.exception("Failed to send scheduled notifications") raise diff --git a/app/config.py b/app/config.py index b3528cb49..e695410d0 100644 --- a/app/config.py +++ b/app/config.py @@ -109,6 +109,11 @@ class Config(object): 'schedule': crontab(minute=1), 'options': {'queue': '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), diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 75acf5152..23a940410 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -483,8 +483,9 @@ def dao_get_scheduled_notifications(): def set_scheduled_notification_to_processed(notification_id): - ScheduledNotification.query.filter( + db.session.query(ScheduledNotification).filter( ScheduledNotification.notification_id == notification_id ).update( {'pending': False} ) + db.session.commit() From 24dfcd2128e7051f351b18012852c8fc50e6faaf Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 23 May 2017 10:43:48 +0100 Subject: [PATCH 17/42] Add normalised_to field to notification --- app/models.py | 1 + .../versions/0086_add_norm_to_notification.py | 21 +++++++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 migrations/versions/0086_add_norm_to_notification.py diff --git a/app/models.py b/app/models.py index 378c45b3e..7ba8ff22c 100644 --- a/app/models.py +++ b/app/models.py @@ -679,6 +679,7 @@ class Notification(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) to = db.Column(db.String, nullable=False) + normalised_to = db.Column(db.String, nullable=True) job_id = db.Column(UUID(as_uuid=True), db.ForeignKey('jobs.id'), index=True, unique=False) job = db.relationship('Job', backref=db.backref('notifications', lazy='dynamic')) job_row_number = db.Column(db.Integer, nullable=True) diff --git a/migrations/versions/0086_add_norm_to_notification.py b/migrations/versions/0086_add_norm_to_notification.py new file mode 100644 index 000000000..346d5b6dc --- /dev/null +++ b/migrations/versions/0086_add_norm_to_notification.py @@ -0,0 +1,21 @@ +""" + +Revision ID: 0086_add_norm_to_notification +Revises: 0085_update_incoming_to_inbound +Create Date: 2017-05-23 10:37:00.404087 + +""" + +from alembic import op +import sqlalchemy as sa + +revision = '0086_add_norm_to_notification' +down_revision = '0085_update_incoming_to_inbound' + + +def upgrade(): + op.add_column('notifications', sa.Column('normalised_to', sa.String(), nullable=True)) + + +def downgrade(): + op.drop_column('notifications', 'normalised_to') From 383dee3bb2c9918a72835601937b6cb42db0dfb1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 24 May 2017 14:52:32 +0100 Subject: [PATCH 18/42] Updated the serialization of Notification.scheduled_for to include minutes. --- app/models.py | 7 ++++--- tests/app/celery/test_scheduled_tasks.py | 6 +++--- tests/app/conftest.py | 2 +- tests/app/dao/test_notification_dao.py | 15 ++++++++------- tests/app/test_utils.py | 6 +++--- .../v2/notifications/test_get_notifications.py | 10 +++++----- 6 files changed, 24 insertions(+), 22 deletions(-) diff --git a/app/models.py b/app/models.py index 76fa31c91..a85972664 100644 --- a/app/models.py +++ b/app/models.py @@ -30,7 +30,7 @@ from app import ( ) from app.history_meta import Versioned -from app.utils import convert_utc_time_in_bst +from app.utils import convert_utc_time_in_bst, convert_bst_to_utc SMS_TYPE = 'sms' EMAIL_TYPE = 'email' @@ -890,8 +890,9 @@ class Notification(db.Model): "created_at": self.created_at.strftime(DATETIME_FORMAT), "sent_at": self.sent_at.strftime(DATETIME_FORMAT) if self.sent_at else None, "completed_at": self.completed_at(), - "scheduled_for": self.scheduled_notification.scheduled_for.strftime( - "%Y-%m-%d %H") if self.scheduled_notification else None + "scheduled_for": convert_bst_to_utc(self.scheduled_notification.scheduled_for + ).strftime( + "%Y-%m-%d %H:%M") if self.scheduled_notification else None } return serialized diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index af8ac54d5..538475b6e 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -424,13 +424,13 @@ def test_should_send_all_scheduled_notifications_to_deliver_queue(notify_db, sample_template, mocker): mocked = mocker.patch('app.celery.provider_tasks.deliver_sms') message_to_deliver = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for="2017-05-01 13") + template=sample_template, scheduled_for="2017-05-01 13:15") sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for="2017-05-01 10", status='delivered') + template=sample_template, scheduled_for="2017-05-01 10:15", status='delivered') sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, template=sample_template) sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for="2017-05-01 14") + template=sample_template, scheduled_for="2017-05-01 14:15") scheduled_notifications = dao_get_scheduled_notifications() assert len(scheduled_notifications) == 1 diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 032a134fc..b7ca092ba 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -494,7 +494,7 @@ def sample_notification( scheduled_notification = ScheduledNotification(id=uuid.uuid4(), notification_id=notification.id, scheduled_for=datetime.strptime(scheduled_for, - "%Y-%m-%d %H")) + "%Y-%m-%d %H:%M")) if status != 'created': scheduled_notification.pending = False db.session.add(scheduled_notification) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index c8e86889c..6d1842da1 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -720,7 +720,7 @@ def test_save_notification_with_no_job(sample_template, mmg_provider): def test_get_notification_by_id(notify_db, notify_db_session, sample_template): notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, template=sample_template, - scheduled_for='2017-05-05 14', + scheduled_for='2017-05-05 14:15', status='created') notification_from_db = get_notification_with_personalisation( sample_template.service.id, @@ -728,7 +728,7 @@ def test_get_notification_by_id(notify_db, notify_db_session, sample_template): key_type=None ) assert notification == notification_from_db - assert notification_from_db.scheduled_notification.scheduled_for == datetime(2017, 5, 5, 14) + assert notification_from_db.scheduled_notification.scheduled_for == datetime(2017, 5, 5, 14, 15) def test_get_notifications_by_reference(notify_db, notify_db_session, sample_service): @@ -1760,20 +1760,21 @@ def test_dao_get_notifications_by_to_field_search_ignores_spaces(sample_template def test_dao_created_scheduled_notification(sample_notification): scheduled_notification = ScheduledNotification(notification_id=sample_notification.id, - scheduled_for=datetime.strptime("2017-01-05 14", "%Y-%m-%d %H")) + scheduled_for=datetime.strptime("2017-01-05 14:15", + "%Y-%m-%d %H:%M")) dao_created_scheduled_notification(scheduled_notification) saved_notification = ScheduledNotification.query.all() assert len(saved_notification) == 1 assert saved_notification[0].notification_id == sample_notification.id - assert saved_notification[0].scheduled_for == datetime(2017, 1, 5, 14) + assert saved_notification[0].scheduled_for == datetime(2017, 1, 5, 14, 15) def test_dao_get_scheduled_notifications(notify_db, notify_db_session, sample_template): notification_1 = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for='2017-05-05 14', + template=sample_template, scheduled_for='2017-05-05 14:15', status='created') sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for='2017-05-04 14', status='delivered') + template=sample_template, scheduled_for='2017-05-04 14:15', status='delivered') sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, template=sample_template, status='created') scheduled_notifications = dao_get_scheduled_notifications() @@ -1784,7 +1785,7 @@ def test_dao_get_scheduled_notifications(notify_db, notify_db_session, sample_te def test_set_scheduled_notification_to_processed(notify_db, notify_db_session, sample_template): notification_1 = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for='2017-05-05 14', + template=sample_template, scheduled_for='2017-05-05 14:15', status='created') scheduled_notifications = dao_get_scheduled_notifications() assert len(scheduled_notifications) == 1 diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 7bb8360af..3b795549f 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -40,7 +40,7 @@ def test_get_utc_in_bst_returns_expected_date(date, expected_date): def test_convert_bst_to_utc(): - bst = "2017-05-12 13" - bst_datetime = datetime.strptime(bst, "%Y-%m-%d %H") + bst = "2017-05-12 13:15" + bst_datetime = datetime.strptime(bst, "%Y-%m-%d %H:%M") utc = convert_bst_to_utc(bst_datetime) - assert utc == datetime(2017, 5, 12, 12, 0) + assert utc == datetime(2017, 5, 12, 12, 15) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 30a35b5aa..2566cbc7d 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -20,12 +20,12 @@ def test_get_notification_by_id_returns_200( ): sample_notification = create_sample_notification( notify_db, notify_db_session, billable_units=billable_units, sent_by=provider, - scheduled_for="2017-05-12 14" + scheduled_for="2017-05-12 15:15" ) another = create_sample_notification( notify_db, notify_db_session, billable_units=billable_units, sent_by=provider, - scheduled_for="2017-06-12 14" + scheduled_for="2017-06-12 15:15" ) auth_header = create_authorization_header(service_id=sample_notification.service_id) response = client.get( @@ -63,7 +63,7 @@ def test_get_notification_by_id_returns_200( "subject": None, 'sent_at': sample_notification.sent_at, 'completed_at': sample_notification.completed_at(), - 'scheduled_for': '2017-05-12 14' + 'scheduled_for': '2017-05-12 14:15' } assert json_response == expected_response @@ -139,7 +139,7 @@ def test_get_notification_by_reference_returns_200(client, notify_db, notify_db_ def test_get_notifications_returns_scheduled_for(client, notify_db, notify_db_session): sample_notification_with_reference = create_sample_notification( - notify_db, notify_db_session, client_reference='some-client-reference', scheduled_for='2017-05-23 16') + notify_db, notify_db_session, client_reference='some-client-reference', scheduled_for='2017-05-23 17:15') auth_header = create_authorization_header(service_id=sample_notification_with_reference.service_id) response = client.get( @@ -153,7 +153,7 @@ def test_get_notifications_returns_scheduled_for(client, notify_db, notify_db_se assert len(json_response['notifications']) == 1 assert json_response['notifications'][0]['id'] == str(sample_notification_with_reference.id) - assert json_response['notifications'][0]['scheduled_for'] == "2017-05-23 16" + assert json_response['notifications'][0]['scheduled_for'] == "2017-05-23 16:15" def test_get_notification_by_reference_nonexistent_reference_returns_no_notifications(client, sample_service): From 554a193cffc4ee0a0dd6b38778b74abe51e3266c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 24 May 2017 16:27:12 +0100 Subject: [PATCH 19/42] separate service deserialization from validation Marshmallow validates and deserialises - BUT, when it deserialises, it explicitly sets `sms_sender=None`, even when you haven't passed sms_sender in. This is problematic, because we wanted to take advantage of sqlalchemy's default value to set sms_sender to `GOVUK` when the actual DB commit happens. Instead, still use marshmallow for validating, but manually carry out the json deserialisation in the model class. This fixes a bug that only manifested when the database was upgraded, but the code hadn't updated. :tada: --- app/models.py | 17 ++++++++++++++++- app/service/rest.py | 23 +++++++++++++++-------- tests/app/service/test_rest.py | 6 +++++- 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/app/models.py b/app/models.py index 8b3b8e053..e6cde1e28 100644 --- a/app/models.py +++ b/app/models.py @@ -188,7 +188,7 @@ class Service(db.Model, Versioned): created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) reply_to_email_address = db.Column(db.Text, index=False, unique=False, nullable=True) letter_contact_block = db.Column(db.Text, index=False, unique=False, nullable=True) - sms_sender = db.Column(db.String(11), nullable=True, default=lambda: current_app.config['FROM_NUMBER']) + sms_sender = db.Column(db.String(11), nullable=False, default=lambda: current_app.config['FROM_NUMBER']) organisation_id = db.Column(UUID(as_uuid=True), db.ForeignKey('organisation.id'), index=True, nullable=True) organisation = db.relationship('Organisation') dvla_organisation_id = db.Column( @@ -215,6 +215,21 @@ class Service(db.Model, Versioned): self.can_send_letters = LETTER_TYPE in [p.permission for p in self.permissions] self.can_send_international_sms = INTERNATIONAL_SMS_TYPE in [p.permission for p in self.permissions] + @classmethod + def from_json(cls, data): + """ + Assumption: data has been validated appropriately. + + Returns a Service object based on the provided data. Deserialises created_by to created_by_id as marshmallow + would. + """ + # validate json with marshmallow + fields = data.copy() + + fields['created_by_id'] = fields.pop('created_by') + + return cls(**fields) + class ServicePermission(db.Model): __tablename__ = "service_permissions" diff --git a/app/service/rest.py b/app/service/rest.py index 180740063..3270b96c2 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -5,11 +5,12 @@ from datetime import datetime from flask import ( jsonify, request, - current_app + current_app, + Blueprint ) from sqlalchemy.orm.exc import NoResultFound -from app.dao import notification_usage_dao +from app.dao import notification_usage_dao, notifications_dao from app.dao.dao_utils import dao_rollback from app.dao.api_key_dao import ( save_model_api_key, @@ -39,11 +40,13 @@ from app.dao.service_whitelist_dao import ( dao_add_and_commit_whitelisted_contacts, dao_remove_service_whitelist ) -from app.dao import notifications_dao from app.dao.provider_statistics_dao import get_fragment_count from app.dao.users_dao import get_user_by_id from app.errors import ( - InvalidRequest, register_errors) + InvalidRequest, + register_errors +) +from app.models import Service from app.service import statistics from app.service.utils import get_whitelist_objects from app.service.sender import send_notification_to_service_users @@ -57,7 +60,6 @@ from app.schemas import ( detailed_service_schema ) from app.utils import pagination_links -from flask import Blueprint service_blueprint = Blueprint('service', __name__) @@ -108,9 +110,14 @@ def create_service(): errors = {'user_id': ['Missing data for required field.']} raise InvalidRequest(errors, status_code=400) - user = get_user_by_id(data['user_id']) - data.pop('user_id', None) - valid_service = service_schema.load(request.get_json()).data + # validate json with marshmallow + service_schema.load(request.get_json()) + + user = get_user_by_id(data.pop('user_id', None)) + + # unpack valid json into service object + valid_service = Service.from_json(data) + dao_create_service(valid_service, user) return jsonify(data=service_schema.dump(valid_service).data), 201 diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 126e9b948..e4929a005 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -20,7 +20,7 @@ from tests.app.conftest import ( sample_notification_history as create_notification_history, sample_notification_with_job ) -from app.models import KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST +from app.models import Service, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST from tests.app.db import create_user @@ -216,6 +216,10 @@ def test_create_service(client, sample_user): assert json_resp['data']['dvla_organisation'] == '001' assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER'] + service_db = Service.query.get(json_resp['data']['id']) + assert service_db.name == 'created service' + assert service_db.sms_sender == current_app.config['FROM_NUMBER'] + auth_header_fetch = create_authorization_header() resp = client.get( From 9f6c037530235e5a47b945ed6ade2ec3de393544 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 24 May 2017 16:27:15 +0100 Subject: [PATCH 20/42] Use iso8601 to validate scheduled_for datetime. Added a validation method that always fails for scheduled notifications. Comment out config for scheduled task. The schedule notifications will be turned on once we can invite services to use it. Waiting for the service permission story, must commit this in order to keep things from going stale. --- app/config.py | 10 ++++---- app/models.py | 3 +-- app/notifications/validators.py | 5 ++++ app/schema_validation/__init__.py | 9 ++++---- app/v2/notifications/post_notifications.py | 7 ++++-- .../notifications/test_get_notifications.py | 4 ++-- .../test_notification_schemas.py | 10 +++++--- .../notifications/test_post_notifications.py | 23 +++++++++++++++++++ 8 files changed, 53 insertions(+), 18 deletions(-) diff --git a/app/config.py b/app/config.py index 5acf396ed..6933ed7d4 100644 --- a/app/config.py +++ b/app/config.py @@ -109,11 +109,11 @@ class Config(object): 'schedule': crontab(minute=1), 'options': {'queue': 'periodic'} }, - 'send-scheduled-notifications': { - 'task': 'send-scheduled-notifications', - 'schedule': crontab(minute='*/15'), - 'options': {'queue': '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), diff --git a/app/models.py b/app/models.py index a85972664..f8d3d6a88 100644 --- a/app/models.py +++ b/app/models.py @@ -891,8 +891,7 @@ class Notification(db.Model): "sent_at": self.sent_at.strftime(DATETIME_FORMAT) if self.sent_at else None, "completed_at": self.completed_at(), "scheduled_for": convert_bst_to_utc(self.scheduled_notification.scheduled_for - ).strftime( - "%Y-%m-%d %H:%M") if self.scheduled_notification else None + ).strftime(DATETIME_FORMAT) if self.scheduled_notification else None } return serialized diff --git a/app/notifications/validators.py b/app/notifications/validators.py index a680f446e..1193d4015 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -90,3 +90,8 @@ def check_sms_content_char_count(content_count): if content_count > char_count_limit: message = 'Content for template has a character count greater than the limit of {}'.format(char_count_limit) raise BadRequestError(message=message) + + +def service_can_schedule_notification(service): + # TODO: implement once the service permission works. + raise BadRequestError(message="Your service must be invited to schedule notifications via the API.") diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 2dff49519..9202458eb 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -1,6 +1,7 @@ import json from datetime import datetime, timedelta +from iso8601 import iso8601, ParseError from jsonschema import (Draft4Validator, ValidationError, FormatChecker) from notifications_utils.recipients import (validate_phone_number, validate_email_address, InvalidPhoneError, InvalidEmailError) @@ -25,14 +26,14 @@ def validate(json_to_validate, schema): def validate_schema_date_with_hour(instance): if isinstance(instance, str): try: - dt = datetime.strptime(instance, "%Y-%m-%d %H:%M") + dt = iso8601.parse_date(instance).replace(tzinfo=None) if dt < datetime.utcnow(): raise ValidationError("datetime can not be in the past") if dt > datetime.utcnow() + timedelta(hours=24): raise ValidationError("datetime can only be 24 hours in the future") - except ValueError as e: - raise ValidationError("datetime format is invalid. Use the format: " - "YYYY-MM-DD HH:MI, for example 2017-05-30 13:15") + except ParseError: + raise ValidationError("datetime format is invalid. It must be a valid ISO8601 date time format, " + "https://en.wikipedia.org/wiki/ISO_8601") return True validator = Draft4Validator(schema, format_checker=format_checker) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index bafa9a372..6cbcbf0f0 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -15,7 +15,7 @@ from app.notifications.validators import ( check_template_is_active, check_sms_content_char_count, validate_and_format_recipient, - check_rate_limiting) + check_rate_limiting, service_can_schedule_notification) from app.schema_validation import validate from app.v2.errors import BadRequestError from app.v2.notifications import v2_notification_blueprint @@ -33,6 +33,10 @@ def post_notification(notification_type): else: form = validate(request.get_json(), post_sms_request) + scheduled_for = form.get("scheduled_for", None) + if scheduled_for: + if not service_can_schedule_notification(authenticated_service): + return check_rate_limiting(authenticated_service, api_user) form_send_to = form['phone_number'] if notification_type == SMS_TYPE else form['email_address'] @@ -57,7 +61,6 @@ def post_notification(notification_type): client_reference=form.get('reference', None), simulated=simulated) - scheduled_for = form.get("scheduled_for", None) if scheduled_for: persist_scheduled_notification(notification.id, form["scheduled_for"]) else: diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 2566cbc7d..bd5cbae6f 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -63,7 +63,7 @@ def test_get_notification_by_id_returns_200( "subject": None, 'sent_at': sample_notification.sent_at, 'completed_at': sample_notification.completed_at(), - 'scheduled_for': '2017-05-12 14:15' + 'scheduled_for': '2017-05-12T14:15:00.000000Z' } assert json_response == expected_response @@ -153,7 +153,7 @@ def test_get_notifications_returns_scheduled_for(client, notify_db, notify_db_se assert len(json_response['notifications']) == 1 assert json_response['notifications'][0]['id'] == str(sample_notification_with_reference.id) - assert json_response['notifications'][0]['scheduled_for'] == "2017-05-23 16:15" + assert json_response['notifications'][0]['scheduled_for'] == "2017-05-23T16:15:00.000000Z" def test_get_notification_by_reference_nonexistent_reference_returns_no_notifications(client, sample_service): diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index bbccd81d5..0aafb38cc 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -371,7 +371,10 @@ def test_post_schema_valid_scheduled_for(schema): @pytest.mark.parametrize("invalid_datetime", - ["2017-05-12 13:00:00", "13:00:00 2017-01-01"]) + ["13:00:00 2017-01-01", + "2017-31-12 13:00:00", + "01-01-2017T14:00:00.0000Z" + ]) @pytest.mark.parametrize("schema", [post_email_request_schema, post_sms_request_schema]) def test_post_email_schema_invalid_scheduled_for(invalid_datetime, schema): @@ -386,8 +389,9 @@ def test_post_email_schema_invalid_scheduled_for(invalid_datetime, schema): error = json.loads(str(e.value)) assert error['status_code'] == 400 assert error['errors'] == [{'error': 'ValidationError', - 'message': "scheduled_for datetime format is invalid. Use the format: " - "YYYY-MM-DD HH:MI, for example 2017-05-30 13:15"}] + 'message': "scheduled_for datetime format is invalid. " + "It must be a valid ISO8601 date time format, " + "https://en.wikipedia.org/wiki/ISO_8601"}] @freeze_time("2017-05-12 13:00:00") diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index e897ab923..3bd191a9b 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -352,6 +352,7 @@ def test_post_sms_should_persist_supplied_sms_number(client, sample_template_wit assert mocked.called +@pytest.mark.skip("Once the service can be invited to schedule notifications we can add this test.") @pytest.mark.parametrize("notification_type, key_send_to, send_to", [("sms", "phone_number", "07700 900 855"), ("email", "email_address", "sample@email.com")]) @@ -374,3 +375,25 @@ def test_post_notification_with_scheduled_for(client, sample_template, sample_em assert len(scheduled_notification) == 1 assert resp_json["id"] == str(scheduled_notification[0].notification_id) assert resp_json["scheduled_for"] == '2017-05-14 14:15' + + +@pytest.mark.parametrize("notification_type, key_send_to, send_to", + [("sms", "phone_number", "07700 900 855"), + ("email", "email_address", "sample@email.com")]) +@freeze_time("2017-05-14 14:00:00") +def test_post_notification_with_scheduled_for_raises_bad_request(client, sample_template, sample_email_template, + notification_type, key_send_to, send_to): + data = { + key_send_to: send_to, + 'template_id': str(sample_email_template.id) if notification_type == 'email' else str(sample_template.id), + 'scheduled_for': '2017-05-14 14:15' + } + auth_header = create_authorization_header(service_id=sample_template.service_id) + + response = client.post('/v2/notifications/{}'.format(notification_type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 400 + error_json = json.loads(response.get_data(as_text=True)) + assert error_json['errors'] == [ + {"error": "BadRequestError", "message": 'Your service must be invited to schedule notifications via the API.'}] From 4b8b6ca91ee8b01c128c8a9f49897beb11c4b018 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 24 May 2017 17:33:22 +0100 Subject: [PATCH 21/42] add test to ensure that updating other things doesnt affect sms sender --- app/models.py | 2 +- tests/app/service/test_rest.py | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index e6cde1e28..4e2b4777b 100644 --- a/app/models.py +++ b/app/models.py @@ -188,7 +188,7 @@ class Service(db.Model, Versioned): created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) reply_to_email_address = db.Column(db.Text, index=False, unique=False, nullable=True) letter_contact_block = db.Column(db.Text, index=False, unique=False, nullable=True) - sms_sender = db.Column(db.String(11), nullable=False, default=lambda: current_app.config['FROM_NUMBER']) + sms_sender = db.Column(db.String(11), nullable=True, default=lambda: current_app.config['FROM_NUMBER']) organisation_id = db.Column(UUID(as_uuid=True), db.ForeignKey('organisation.id'), index=True, nullable=True) organisation = db.relationship('Organisation') dvla_organisation_id = db.Column( diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index e4929a005..cbf3eb1b2 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1735,3 +1735,19 @@ def test_update_service_does_not_call_send_notification_when_restricted_not_chan assert resp.status_code == 200 assert not send_notification_mock.called + + +def test_update_service_works_when_sms_sender_is_null(sample_service, client, mocker): + sample_service.sms_sender = None + data = {'name': 'new name'} + + resp = client.post( + 'service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[create_authorization_header()], + content_type='application/json' + ) + + assert resp.status_code == 200 + # make sure it wasn't changed to not-null under the hood + assert sample_service.sms_sender is None From f555c7a73b41619a220fa5a1cc9d848e00d2099b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 25 May 2017 11:41:07 +0100 Subject: [PATCH 22/42] Refactor tests to use the create_notication in tests.app.db --- tests/app/celery/test_scheduled_tasks.py | 19 +-- tests/app/db.py | 15 ++- .../notifications/test_get_notifications.py | 110 +++++++++--------- .../notifications/test_post_notifications.py | 64 +++++----- 4 files changed, 103 insertions(+), 105 deletions(-) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 538475b6e..194d764fd 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -32,8 +32,7 @@ from tests.app.db import create_notification, create_service from tests.app.conftest import ( sample_job as create_sample_job, sample_notification_history as create_notification_history, - create_custom_template, - sample_notification) + create_custom_template) from tests.conftest import set_config_values from unittest.mock import call, patch, PropertyMock @@ -419,18 +418,12 @@ def test_switch_providers_on_slow_delivery_does_not_switch_based_on_older_notifi @freeze_time("2017-05-01 14:00:00") -def test_should_send_all_scheduled_notifications_to_deliver_queue(notify_db, - notify_db_session, - sample_template, mocker): +def test_should_send_all_scheduled_notifications_to_deliver_queue(sample_template, mocker): mocked = mocker.patch('app.celery.provider_tasks.deliver_sms') - message_to_deliver = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for="2017-05-01 13:15") - sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for="2017-05-01 10:15", status='delivered') - sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template) - sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - template=sample_template, scheduled_for="2017-05-01 14:15") + message_to_deliver = create_notification(template=sample_template, scheduled_for="2017-05-01 13:15") + create_notification(template=sample_template, scheduled_for="2017-05-01 10:15", status='delivered') + create_notification(template=sample_template) + create_notification(template=sample_template, scheduled_for="2017-05-01 14:15") scheduled_notifications = dao_get_scheduled_notifications() assert len(scheduled_notifications) == 1 diff --git a/tests/app/db.py b/tests/app/db.py index d418b1c3b..afb35e252 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -3,9 +3,9 @@ import uuid from app.dao.jobs_dao import dao_create_job from app.models import (Service, User, Template, Notification, EMAIL_TYPE, LETTER_TYPE, - SMS_TYPE, KEY_TYPE_NORMAL, Job, ServicePermission) + SMS_TYPE, KEY_TYPE_NORMAL, Job, ServicePermission, ScheduledNotification) from app.dao.users_dao import save_model_user -from app.dao.notifications_dao import dao_create_notification +from app.dao.notifications_dao import dao_create_notification, dao_created_scheduled_notification from app.dao.templates_dao import dao_create_template from app.dao.services_dao import dao_create_service from app.dao.service_permissions_dao import dao_add_service_permission @@ -80,7 +80,8 @@ def create_notification( client_reference=None, rate_multiplier=None, international=False, - phone_prefix=None + phone_prefix=None, + scheduled_for=None ): if created_at is None: created_at = datetime.utcnow() @@ -118,6 +119,14 @@ def create_notification( } notification = Notification(**data) dao_create_notification(notification) + if scheduled_for: + scheduled_notification = ScheduledNotification(id=uuid.uuid4(), + notification_id=notification.id, + scheduled_for=datetime.strptime(scheduled_for, + "%Y-%m-%d %H:%M")) + if status != 'created': + scheduled_notification.pending = False + dao_created_scheduled_notification(scheduled_notification) return notification diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index bd5cbae6f..ff487275c 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -4,10 +4,10 @@ from flask import json from app import DATETIME_FORMAT from tests import create_authorization_header -from tests.app.conftest import ( - sample_notification as create_sample_notification, - sample_template as create_sample_template -) +from tests.app.db import ( + create_notification, + create_template, + create_service) @pytest.mark.parametrize('billable_units, provider', [ @@ -16,17 +16,15 @@ from tests.app.conftest import ( (1, None) ]) def test_get_notification_by_id_returns_200( - client, notify_db, notify_db_session, billable_units, provider + client, billable_units, provider, sample_template ): - sample_notification = create_sample_notification( - notify_db, notify_db_session, billable_units=billable_units, sent_by=provider, - scheduled_for="2017-05-12 15:15" - ) + sample_notification = create_notification(template=sample_template, billable_units=billable_units, sent_by=provider, + scheduled_for="2017-05-12 15:15" + ) - another = create_sample_notification( - notify_db, notify_db_session, billable_units=billable_units, sent_by=provider, - scheduled_for="2017-06-12 15:15" - ) + another = create_notification(template=sample_template, billable_units=billable_units, sent_by=provider, + scheduled_for="2017-06-12 15:15" + ) auth_header = create_authorization_header(service_id=sample_notification.service_id) response = client.get( path='/v2/notifications/{}'.format(sample_notification.id), @@ -70,11 +68,11 @@ def test_get_notification_by_id_returns_200( def test_get_notification_by_id_with_placeholders_returns_200( - client, notify_db, notify_db_session, sample_email_template_with_placeholders + client, sample_email_template_with_placeholders ): - sample_notification = create_sample_notification( - notify_db, notify_db_session, template=sample_email_template_with_placeholders, personalisation={"name": "Bob"} - ) + sample_notification = create_notification(template=sample_email_template_with_placeholders, + personalisation={"name": "Bob"} + ) auth_header = create_authorization_header(service_id=sample_notification.service_id) response = client.get( @@ -118,9 +116,9 @@ def test_get_notification_by_id_with_placeholders_returns_200( assert json_response == expected_response -def test_get_notification_by_reference_returns_200(client, notify_db, notify_db_session): - sample_notification_with_reference = create_sample_notification( - notify_db, notify_db_session, client_reference='some-client-reference') +def test_get_notification_by_reference_returns_200(client, sample_template): + sample_notification_with_reference = create_notification(template=sample_template, + client_reference='some-client-reference') auth_header = create_authorization_header(service_id=sample_notification_with_reference.service_id) response = client.get( @@ -137,9 +135,10 @@ def test_get_notification_by_reference_returns_200(client, notify_db, notify_db_ assert json_response['notifications'][0]['reference'] == "some-client-reference" -def test_get_notifications_returns_scheduled_for(client, notify_db, notify_db_session): - sample_notification_with_reference = create_sample_notification( - notify_db, notify_db_session, client_reference='some-client-reference', scheduled_for='2017-05-23 17:15') +def test_get_notifications_returns_scheduled_for(client, sample_template): + sample_notification_with_reference = create_notification(template=sample_template, + client_reference='some-client-reference', + scheduled_for='2017-05-23 17:15') auth_header = create_authorization_header(service_id=sample_notification_with_reference.service_id) response = client.get( @@ -208,8 +207,8 @@ def test_get_notification_by_id_invalid_id(client, sample_notification, id): } -def test_get_all_notifications_returns_200(client, notify_db, notify_db_session): - notifications = [create_sample_notification(notify_db, notify_db_session) for _ in range(2)] +def test_get_all_notifications_returns_200(client, sample_template): + notifications = [create_notification(template=sample_template) for _ in range(2)] notification = notifications[-1] auth_header = create_authorization_header(service_id=notification.service_id) @@ -252,13 +251,13 @@ def test_get_all_notifications_no_notifications_if_no_notifications(client, samp assert len(json_response['notifications']) == 0 -def test_get_all_notifications_filter_by_template_type(client, notify_db, notify_db_session): - email_template = create_sample_template(notify_db, notify_db_session, template_type="email") - sms_template = create_sample_template(notify_db, notify_db_session, template_type="sms") +def test_get_all_notifications_filter_by_template_type(client): + service = create_service() + email_template = create_template(service=service, template_type="email") + sms_template = create_template(service=service, template_type="sms") - notification = create_sample_notification( - notify_db, notify_db_session, template=email_template, to_field="don.draper@scdp.biz") - create_sample_notification(notify_db, notify_db_session, template=sms_template) + notification = create_notification(template=email_template, to_field="don.draper@scdp.biz") + create_notification(template=sms_template) auth_header = create_authorization_header(service_id=notification.service_id) response = client.get( @@ -300,9 +299,9 @@ def test_get_all_notifications_filter_by_template_type_invalid_template_type(cli assert json_response['errors'][0]['message'] == "template_type orange is not one of [sms, email, letter]" -def test_get_all_notifications_filter_by_single_status(client, notify_db, notify_db_session): - notification = create_sample_notification(notify_db, notify_db_session, status="pending") - create_sample_notification(notify_db, notify_db_session) +def test_get_all_notifications_filter_by_single_status(client, sample_template): + notification = create_notification(template=sample_template, status="pending") + create_notification(template=sample_template) auth_header = create_authorization_header(service_id=notification.service_id) response = client.get( @@ -338,12 +337,12 @@ def test_get_all_notifications_filter_by_status_invalid_status(client, sample_no "delivered, pending, failed, technical-failure, temporary-failure, permanent-failure]" -def test_get_all_notifications_filter_by_multiple_statuses(client, notify_db, notify_db_session): +def test_get_all_notifications_filter_by_multiple_statuses(client, sample_template): notifications = [ - create_sample_notification(notify_db, notify_db_session, status=_status) + create_notification(template=sample_template, status=_status) for _status in ["created", "pending", "sending"] ] - failed_notification = create_sample_notification(notify_db, notify_db_session, status="permanent-failure") + failed_notification = create_notification(template=sample_template, status="permanent-failure") auth_header = create_authorization_header(service_id=notifications[0].service_id) response = client.get( @@ -365,10 +364,10 @@ def test_get_all_notifications_filter_by_multiple_statuses(client, notify_db, no assert failed_notification.id not in returned_notification_ids -def test_get_all_notifications_filter_by_failed_status(client, notify_db, notify_db_session): - created_notification = create_sample_notification(notify_db, notify_db_session, status="created") +def test_get_all_notifications_filter_by_failed_status(client, sample_template): + created_notification = create_notification(template=sample_template, status="created") failed_notifications = [ - create_sample_notification(notify_db, notify_db_session, status=_status) + create_notification(template=sample_template, status=_status) for _status in ["technical-failure", "temporary-failure", "permanent-failure"] ] @@ -392,9 +391,9 @@ def test_get_all_notifications_filter_by_failed_status(client, notify_db, notify assert created_notification.id not in returned_notification_ids -def test_get_all_notifications_filter_by_id(client, notify_db, notify_db_session): - older_notification = create_sample_notification(notify_db, notify_db_session) - newer_notification = create_sample_notification(notify_db, notify_db_session) +def test_get_all_notifications_filter_by_id(client, sample_template): + older_notification = create_notification(template=sample_template) + newer_notification = create_notification(template=sample_template) auth_header = create_authorization_header(service_id=newer_notification.service_id) response = client.get( @@ -425,8 +424,8 @@ def test_get_all_notifications_filter_by_id_invalid_id(client, sample_notificati assert json_response['errors'][0]['message'] == "older_than is not a valid UUID" -def test_get_all_notifications_filter_by_id_no_notifications_if_nonexistent_id(client, notify_db, notify_db_session): - notification = create_sample_notification(notify_db, notify_db_session) +def test_get_all_notifications_filter_by_id_no_notifications_if_nonexistent_id(client, sample_template): + notification = create_notification(template=sample_template) auth_header = create_authorization_header(service_id=notification.service_id) response = client.get( @@ -443,8 +442,8 @@ def test_get_all_notifications_filter_by_id_no_notifications_if_nonexistent_id(c assert len(json_response['notifications']) == 0 -def test_get_all_notifications_filter_by_id_no_notifications_if_last_notification(client, notify_db, notify_db_session): - notification = create_sample_notification(notify_db, notify_db_session) +def test_get_all_notifications_filter_by_id_no_notifications_if_last_notification(client, sample_template): + notification = create_notification(template=sample_template) auth_header = create_authorization_header(service_id=notification.service_id) response = client.get( @@ -460,23 +459,22 @@ def test_get_all_notifications_filter_by_id_no_notifications_if_last_notificatio assert len(json_response['notifications']) == 0 -def test_get_all_notifications_filter_multiple_query_parameters(client, notify_db, notify_db_session): - email_template = create_sample_template(notify_db, notify_db_session, template_type="email") - +def test_get_all_notifications_filter_multiple_query_parameters(client, sample_email_template): # this is the notification we are looking for - older_notification = create_sample_notification( - notify_db, notify_db_session, template=email_template, status="pending") + older_notification = create_notification( + template=sample_email_template, status="pending") # wrong status - create_sample_notification(notify_db, notify_db_session, template=email_template) + create_notification(template=sample_email_template) + wrong_template = create_template(sample_email_template.service, template_type='sms') # wrong template - create_sample_notification(notify_db, notify_db_session, status="pending") + create_notification(template=wrong_template, status="pending") # we only want notifications created before this one - newer_notification = create_sample_notification(notify_db, notify_db_session) + newer_notification = create_notification(template=sample_email_template) # this notification was created too recently - create_sample_notification(notify_db, notify_db_session, template=email_template, status="pending") + create_notification(template=sample_email_template, status="pending") auth_header = create_authorization_header(service_id=newer_notification.service_id) response = client.get( diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 3bd191a9b..afce12947 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -13,40 +13,38 @@ from tests.app.conftest import sample_template as create_sample_template, sample @pytest.mark.parametrize("reference", [None, "reference_from_client"]) -def test_post_sms_notification_returns_201(notify_api, sample_template_with_placeholders, mocker, reference): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - data = { - 'phone_number': '+447700900855', - 'template_id': str(sample_template_with_placeholders.id), - 'personalisation': {' Name': 'Jo'} - } - if reference: - data.update({"reference": reference}) - auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) +def test_post_sms_notification_returns_201(client, sample_template_with_placeholders, mocker, reference): + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = { + 'phone_number': '+447700900855', + 'template_id': str(sample_template_with_placeholders.id), + 'personalisation': {' Name': 'Jo'} + } + if reference: + data.update({"reference": reference}) + auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) - response = client.post( - path='/v2/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 201 - resp_json = json.loads(response.get_data(as_text=True)) - notifications = Notification.query.all() - assert len(notifications) == 1 - notification_id = notifications[0].id - assert resp_json['id'] == str(notification_id) - assert resp_json['reference'] == reference - assert resp_json['content']['body'] == sample_template_with_placeholders.content.replace("(( Name))", "Jo") - assert resp_json['content']['from_number'] == current_app.config['FROM_NUMBER'] - assert 'v2/notifications/{}'.format(notification_id) in resp_json['uri'] - assert resp_json['template']['id'] == str(sample_template_with_placeholders.id) - assert resp_json['template']['version'] == sample_template_with_placeholders.version - assert 'services/{}/templates/{}'.format(sample_template_with_placeholders.service_id, - sample_template_with_placeholders.id) \ - in resp_json['template']['uri'] - assert not resp_json["scheduled_for"] - assert mocked.called + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True)) + notifications = Notification.query.all() + assert len(notifications) == 1 + notification_id = notifications[0].id + assert resp_json['id'] == str(notification_id) + assert resp_json['reference'] == reference + assert resp_json['content']['body'] == sample_template_with_placeholders.content.replace("(( Name))", "Jo") + assert resp_json['content']['from_number'] == current_app.config['FROM_NUMBER'] + assert 'v2/notifications/{}'.format(notification_id) in resp_json['uri'] + assert resp_json['template']['id'] == str(sample_template_with_placeholders.id) + assert resp_json['template']['version'] == sample_template_with_placeholders.version + assert 'services/{}/templates/{}'.format(sample_template_with_placeholders.service_id, + sample_template_with_placeholders.id) \ + in resp_json['template']['uri'] + assert not resp_json["scheduled_for"] + assert mocked.called @pytest.mark.parametrize("notification_type, key_send_to, send_to", From 586c83a2c72637af611b4a9855e7a057c48163e2 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 25 May 2017 13:33:26 +0100 Subject: [PATCH 23/42] Add a script to delete sqs queues: * Uses boto to retrieve/delete queues * Additional functions to output/read from csv --- scripts/delete_sqs_queues.py | 84 ++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 scripts/delete_sqs_queues.py diff --git a/scripts/delete_sqs_queues.py b/scripts/delete_sqs_queues.py new file mode 100644 index 000000000..bdbf6ff65 --- /dev/null +++ b/scripts/delete_sqs_queues.py @@ -0,0 +1,84 @@ +import boto3 +import csv +from datetime import datetime +from pprint import pprint +import os + +client = boto3.client('sqs', region_name=os.getenv('AWS_REGION')) + + +def _formatted_date_from_timestamp(timestamp): + return datetime.fromtimestamp( + int(timestamp) + ).strftime('%Y-%m-%d %H:%M:%S') + + +def get_queues(): + response = client.list_queues() + queues = response['QueueUrls'] + return queues + + +def get_queue_attributes(queue_name): + response = client.get_queue_attributes( + QueueUrl=queue_name, + AttributeNames=[ + 'All' + ] + ) + queue_attributes = response['Attributes'] + return queue_attributes + + +def delete_queue(queue_name): + response = client.delete_queue( + QueueUrl=queue_name + ) + if response['ResponseMetadata']['HTTPStatusCode'] == 200: + print('Deleted queue successfully') + else: + print('Error occured when attempting to delete queue') + pprint(response) + return response + + +def output_to_csv(queue_attributes): + csv_name = 'queues.csv' + with open(csv_name, 'w') as csvfile: + fieldnames = [ + 'Queue Name', + 'Number of Messages', + 'Number of Messages Delayed', + 'Number of Messages Not Visible', + 'Created' + ] + writer = csv.DictWriter(csvfile, fieldnames=fieldnames) + writer.writeheader() + for queue_attr in queue_attributes: + queue_url = client.get_queue_url( + QueueName=queue_attr['QueueArn'] + )['QueueUrl'] + writer.writerow({ + 'Queue Name': queue_attr['QueueArn'], + 'Queue URL': queue_url, + 'Number of Messages': queue_attr['ApproximateNumberOfMessages'], + 'Number of Messages Delayed': queue_attr['ApproximateNumberOfMessagesDelayed'], + 'Number of Messages Not Visible': queue_attr['ApproximateNumberOfMessagesNotVisible'], + 'Created': _formatted_date_from_timestamp(queue_attr['CreatedTimestamp']) + }) + return csv_name + + +def read_from_csv(csv_name): + queue_urls = [] + with open(csv_name, 'r') as csvfile: + next(csvfile) + rows = csv.reader(csvfile, delimiter=',') + for row in rows: + queue_urls.append(row[1]) + return queue_urls + + +queues = get_queues() +for queue in queues: + delete_queue(queue) From 6ad36b274b9b035edcf856f1036dab0806c90951 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 25 May 2017 13:46:03 +0100 Subject: [PATCH 24/42] Fix merge conflict with migration file --- ...d_notifications.py => 0087_scheduled_notifications.py} | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) rename migrations/versions/{0086_scheduled_notifications.py => 0087_scheduled_notifications.py} (85%) diff --git a/migrations/versions/0086_scheduled_notifications.py b/migrations/versions/0087_scheduled_notifications.py similarity index 85% rename from migrations/versions/0086_scheduled_notifications.py rename to migrations/versions/0087_scheduled_notifications.py index 58e1d5a55..9066e8ac1 100644 --- a/migrations/versions/0086_scheduled_notifications.py +++ b/migrations/versions/0087_scheduled_notifications.py @@ -1,7 +1,7 @@ """empty message -Revision ID: 0086_scheduled_notifications -Revises: 0085_update_incoming_to_inbound +Revision ID: 0087_scheduled_notifications +Revises: 0086_add_norm_to_notification Create Date: 2017-05-15 12:50:20.041950 """ @@ -9,8 +9,8 @@ from alembic import op import sqlalchemy as sa from sqlalchemy.dialects import postgresql -revision = '0086_scheduled_notifications' -down_revision = '0085_update_incoming_to_inbound' +revision = '0087_scheduled_notifications' +down_revision = '0086_add_norm_to_notification' def upgrade(): From 1f591d3490795cc26e35da97f602be0103aff61b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 25 May 2017 16:13:33 +0100 Subject: [PATCH 25/42] Add iso8601 to requirements --- requirements.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements.txt b/requirements.txt index fe25533c1..3551937fa 100644 --- a/requirements.txt +++ b/requirements.txt @@ -20,6 +20,7 @@ jsonschema==2.5.1 gunicorn==19.6.0 docopt==0.6.2 six==1.10.0 +iso8601==0.1.11 # pin to minor version 3.1.x notifications-python-client>=3.1,<3.2 From 8e3e31faafcd6d94910b2f058d6aa4bb1f4f5f30 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 22 May 2017 11:33:24 +0100 Subject: [PATCH 26/42] Updated service DAO and API end points --- app/dao/services_dao.py | 12 +- app/schemas.py | 56 +++++-- app/service/rest.py | 3 +- tests/app/dao/test_services_dao.py | 38 +++++ tests/app/service/test_rest.py | 243 ++++++++++++++++++++++------- 5 files changed, 280 insertions(+), 72 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 0e0701560..f1c66126a 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -138,7 +138,7 @@ def dao_create_service(service, user, service_id=None, service_permissions=[SMS_ for permission in service_permissions: service_permission = ServicePermission(service_id=service.id, permission=permission) - db.session.add(service_permission) + service.permissions.append(service_permission) db.session.add(service) @@ -149,6 +149,16 @@ def dao_update_service(service): db.session.add(service) +@transactional +@version_class(Service) +def dao_remove_service_permission(service, permission): + for p in service.permissions: + if p.permission == permission: + service.permissions.remove(p) + + db.session.add(service) + + def dao_add_user_to_service(service, user, permissions=None): permissions = permissions or [] try: diff --git a/app/schemas.py b/app/schemas.py index aa8980865..f673cc7fa 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -25,6 +25,7 @@ from notifications_utils.recipients import ( from app import ma from app import models +from app.models import ServicePermission from app.dao.permissions_dao import permission_dao from app.utils import get_template_instance @@ -178,18 +179,34 @@ class ServiceSchema(BaseSchema): organisation = field_for(models.Service, 'organisation') branding = field_for(models.Service, 'branding') dvla_organisation = field_for(models.Service, 'dvla_organisation') + permissions = fields.Method("service_permissions") + + def service_permissions(self, service): + permissions = [] + + from app.dao.service_permissions_dao import dao_fetch_service_permissions + perms = dao_fetch_service_permissions(service.id) + for p in perms: + permission = { + "service_id": service.id, + "permission": p.permission + } + permissions.append(permission) + return permissions class Meta: model = models.Service - exclude = ('updated_at', - 'created_at', - 'api_keys', - 'templates', - 'jobs', - 'old_id', - 'template_statistics', - 'service_provider_stats', - 'service_notification_stats') + exclude = ( + 'updated_at', + 'created_at', + 'api_keys', + 'templates', + 'jobs', + 'old_id', + 'template_statistics', + 'service_provider_stats', + 'service_notification_stats', + ) strict = True @validates('sms_sender') @@ -197,6 +214,27 @@ class ServiceSchema(BaseSchema): if value and not re.match(r'^[a-zA-Z0-9\s]+$', value): raise ValidationError('Only alphanumeric characters allowed') + @validates('permissions') + def validate_permissions(self, value): + for v in [val.permission for val in value]: + if v not in models.SERVICE_PERMISSION_TYPES: + raise ValidationError("Invalid Service Permission: '{}'".format(v)) + + @pre_load() + def format_permissions_for_data_model(self, in_data): + if isinstance(in_data, dict) and 'permissions' in in_data: + permissions = [] + for p in in_data.get('permissions'): + permission = models.ServicePermission(service_id=in_data["id"], permission=p) + permissions.append(permission) + in_data['permissions'] = permissions + + @post_dump + def format_permissions_as_string_array(self, in_data): + if isinstance(in_data, dict) and 'permissions' in in_data: + in_data['permissions'] = [p.get('permission') for p in in_data.get('permissions')] + return in_data + class DetailedServiceSchema(BaseSchema): statistics = fields.Dict() diff --git a/app/service/rest.py b/app/service/rest.py index 3270b96c2..c170420f9 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -95,12 +95,11 @@ def get_services(): def get_service_by_id(service_id): if request.args.get('detailed') == 'True': data = get_detailed_service(service_id, today_only=request.args.get('today_only') == 'True') - return jsonify(data=data) else: fetched = dao_fetch_service_by_id(service_id) data = service_schema.dump(fetched).data - return jsonify(data=data) + return jsonify(data=data) @service_blueprint.route('', methods=['POST']) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index ff7b3b54b..0c78a7940 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -11,6 +11,7 @@ from app.dao.services_dao import ( dao_create_service, dao_add_user_to_service, dao_remove_user_from_service, + dao_remove_service_permission as dao_services_remove_service_permission, dao_fetch_all_services, dao_fetch_service_by_id, dao_fetch_all_services_by_user, @@ -45,7 +46,10 @@ from app.models import ( InvitedUser, Service, ServicePermission, +<<<<<<< HEAD ServicePermissionTypes, +======= +>>>>>>> Updated service DAO and API end points BRANDING_GOVUK, DVLA_ORG_HM_GOVERNMENT, KEY_TYPE_NORMAL, @@ -372,6 +376,40 @@ def test_update_service_creates_a_history_record_with_current_data(sample_user): assert Service.get_history_model().query.filter_by(name='updated_service_name').one().version == 2 +def test_update_service_permission_creates_a_history_record_with_current_data(sample_user): + assert Service.query.count() == 0 + assert Service.get_history_model().query.count() == 0 + service = Service(name="service_name", + email_from="email_from", + message_limit=1000, + restricted=False, + created_by=sample_user) + dao_create_service(service, sample_user) + + service.permissions.append(ServicePermission(service_id=service.id, permission='letter')) + dao_update_service(service) + + assert Service.query.count() == 1 + assert Service.get_history_model().query.count() == 2 + + service_from_db = Service.query.first() + + assert service_from_db.version == 2 + assert LETTER_TYPE in [p.permission for p in service_from_db.permissions] + + dao_services_remove_service_permission(service, permission='sms') + + assert Service.query.count() == 1 + assert Service.get_history_model().query.count() == 3 + + service_from_db = Service.query.first() + assert service_from_db.version == 3 + assert SMS_TYPE not in [p.permission for p in service_from_db.permissions] + + assert len(Service.get_history_model().query.filter_by(name='service_name').all()) == 3 + assert Service.get_history_model().query.filter_by(name='service_name').all()[2].version == 3 + + def test_create_service_and_history_is_transactional(sample_user): assert Service.query.count() == 0 assert Service.get_history_model().query.count() == 0 diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index cbf3eb1b2..33016d7fb 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -10,7 +10,7 @@ from freezegun import freeze_time from app.dao.users_dao import save_model_user from app.dao.services_dao import dao_remove_user_from_service -from app.models import User, Organisation, DVLA_ORG_LAND_REGISTRY, Rate +from app.models import User, Organisation, DVLA_ORG_LAND_REGISTRY, Rate, ServicePermission from tests import create_authorization_header from tests.app.db import create_template from tests.app.conftest import ( @@ -20,28 +20,26 @@ from tests.app.conftest import ( sample_notification_history as create_notification_history, sample_notification_with_job ) -from app.models import Service, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST +from app.models import KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE from tests.app.db import create_user -def test_get_service_list(notify_api, service_factory): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - service_factory.get('one') - service_factory.get('two') - service_factory.get('three') - auth_header = create_authorization_header() - response = client.get( - '/service', - headers=[auth_header] - ) - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 3 - assert json_resp['data'][0]['name'] == 'one' - assert json_resp['data'][1]['name'] == 'two' - assert json_resp['data'][2]['name'] == 'three' +def test_get_service_list(client, service_factory): + service_factory.get('one') + service_factory.get('two') + service_factory.get('three') + auth_header = create_authorization_header() + response = client.get( + '/service', + headers=[auth_header] + ) + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 3 + assert json_resp['data'][0]['name'] == 'one' + assert json_resp['data'][1]['name'] == 'two' + assert json_resp['data'][2]['name'] == 'three' def test_get_service_list_with_only_active_flag(client, service_factory): @@ -117,17 +115,15 @@ def test_get_service_list_by_user_should_return_empty_list_if_no_services(client assert len(json_resp['data']) == 0 -def test_get_service_list_should_return_empty_list_if_no_services(notify_api, notify_db, notify_db_session): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - response = client.get( - '/service', - headers=[auth_header] - ) - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 0 +def test_get_service_list_should_return_empty_list_if_no_services(client): + auth_header = create_authorization_header() + response = client.get( + '/service', + headers=[auth_header] + ) + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 0 def test_get_service_by_id(client, sample_service): @@ -147,6 +143,32 @@ def test_get_service_by_id(client, sample_service): assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER'] +def test_get_service_list_has_default_permissions(client, service_factory): + service_factory.get('one') + service_factory.get('two') + service_factory.get('three') + auth_header = create_authorization_header() + response = client.get( + '/service', + headers=[auth_header] + ) + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 3 + assert all([set(json['permissions']) == set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']]) + + +def test_get_service_by_id_has_default_permissions(client, sample_service): + auth_header = create_authorization_header() + resp = client.get( + '/service/{}'.format(sample_service.id), + headers=[auth_header] + ) + json_resp = json.loads(resp.get_data(as_text=True)) + + assert set(json_resp['data']['permissions']) == set([EMAIL_TYPE, SMS_TYPE]) + + def test_get_service_by_id_should_404_if_no_service(notify_api, notify_db): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -414,39 +436,140 @@ def test_update_service(client, notify_db, sample_service): assert result['data']['dvla_organisation'] == DVLA_ORG_LAND_REGISTRY -def test_update_service_flags(notify_api, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - resp = client.get( - '/service/{}'.format(sample_service.id), - headers=[auth_header] - ) - json_resp = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == 200 - assert json_resp['data']['name'] == sample_service.name - assert json_resp['data']['research_mode'] is False - assert json_resp['data']['can_send_letters'] is False - assert json_resp['data']['can_send_international_sms'] is False +def test_update_service_flags(client, sample_service): + auth_header = create_authorization_header() + resp = client.get( + '/service/{}'.format(sample_service.id), + headers=[auth_header] + ) + json_resp = json.loads(resp.get_data(as_text=True)) + assert resp.status_code == 200 + assert json_resp['data']['name'] == sample_service.name + assert json_resp['data']['research_mode'] is False + assert json_resp['data']['can_send_letters'] is False + assert json_resp['data']['can_send_international_sms'] is False - data = { - 'research_mode': True, - 'can_send_letters': True, - 'can_send_international_sms': True, - } + data = { + 'research_mode': True, + 'can_send_letters': True, + 'can_send_international_sms': True, + } - auth_header = create_authorization_header() + auth_header = create_authorization_header() - resp = client.post( - '/service/{}'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header] - ) - result = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == 200 - assert result['data']['research_mode'] is True - assert result['data']['can_send_letters'] is True - assert result['data']['can_send_international_sms'] is True + resp = client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + result = json.loads(resp.get_data(as_text=True)) + assert resp.status_code == 200 + assert result['data']['research_mode'] is True + assert result['data']['can_send_letters'] is True + assert result['data']['can_send_international_sms'] is True + + +def test_update_service_flags_will_add_service_permissions(client, sample_service): + auth_header = create_authorization_header() + data = { + 'research_mode': True, + 'can_send_letters': True, + 'can_send_international_sms': True, + } + + resp = client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + result = json.loads(resp.get_data(as_text=True)) + + assert resp.status_code == 200 + assert result['data']['research_mode'] is True + assert result['data']['can_send_letters'] is True + assert result['data']['can_send_international_sms'] is True + + +def test_update_permissions_can_add_service_permissions(client, sample_service): + from app.models import ServicePermission + auth_header = create_authorization_header() + + data = { + 'research_mode': True, + 'can_send_letters': True, + 'can_send_international_sms': True, + 'permissions': [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE] + } + + resp = client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + result = json.loads(resp.get_data(as_text=True)) + + assert resp.status_code == 200 + assert result['data']['research_mode'] is True + assert result['data']['can_send_letters'] is True + assert result['data']['can_send_international_sms'] is True + assert all(set(result['data']['permissions']) & set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE])) + + +def test_update_permissions_with_an_invalid_permission_will_raise_error(client, sample_service): + auth_header = create_authorization_header() + invalid_permission = 'invalid_permission' + + data = { + 'research_mode': True, + 'can_send_letters': True, + 'can_send_international_sms': True, + 'permissions': [EMAIL_TYPE, SMS_TYPE, invalid_permission] + } + + resp = client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + result = json.loads(resp.get_data(as_text=True)) + + assert resp.status_code == 400 + assert result['result'] == 'error' + assert "Invalid Service Permission: '{}'".format(invalid_permission) in result['message']['permissions'] + + +def test_update_service_flags_can_remove_service_permissions(client, sample_service): + auth_header = create_authorization_header() + initial_data = { + 'research_mode': True, + 'can_send_letters': True, + 'can_send_international_sms': True, + 'permissions': [EMAIL_TYPE, SMS_TYPE] + } + + resp = client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(initial_data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + remove_permission_data = { + 'can_send_letters': True, + 'can_send_international_sms': False, + 'permissions': [EMAIL_TYPE] + } + + resp = client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(remove_permission_data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + result = json.loads(resp.get_data(as_text=True)) + + assert resp.status_code == 200 + assert result['data']['can_send_letters'] is True + assert result['data']['can_send_international_sms'] is False + assert result['data']['permissions'] == [EMAIL_TYPE] def test_update_service_research_mode_throws_validation_error(notify_api, sample_service): From e59a1ab10441aaa9bece4d6173b7755842109bb4 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 22 May 2017 13:44:42 +0100 Subject: [PATCH 27/42] Update tests for existing flags to set service permissions --- tests/app/service/test_rest.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 33016d7fb..239c833fd 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -20,7 +20,9 @@ from tests.app.conftest import ( sample_notification_history as create_notification_history, sample_notification_with_job ) -from app.models import KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE +from app.models import ( + KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE +) from tests.app.db import create_user @@ -155,7 +157,7 @@ def test_get_service_list_has_default_permissions(client, service_factory): assert response.status_code == 200 json_resp = json.loads(response.get_data(as_text=True)) assert len(json_resp['data']) == 3 - assert all([set(json['permissions']) == set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']]) + assert all([set(json['permissions']) & set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']]) def test_get_service_by_id_has_default_permissions(client, sample_service): @@ -488,6 +490,7 @@ def test_update_service_flags_will_add_service_permissions(client, sample_servic assert result['data']['research_mode'] is True assert result['data']['can_send_letters'] is True assert result['data']['can_send_international_sms'] is True + assert all(set(result['data']['permissions']) & set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE])) def test_update_permissions_can_add_service_permissions(client, sample_service): From 2f626fa6fca65fc3f989a6d91a6da3b63c440600 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 22 May 2017 19:03:59 +0100 Subject: [PATCH 28/42] Add permissions validation in service schema --- app/schemas.py | 20 ++++++++++++------ app/service/rest.py | 5 ++++- tests/app/service/test_rest.py | 38 ++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 7 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index f673cc7fa..dd9e0d0ae 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -25,8 +25,9 @@ from notifications_utils.recipients import ( from app import ma from app import models -from app.models import ServicePermission +from app.models import ServicePermission, INTERNATIONAL_SMS_TYPE, SMS_TYPE from app.dao.permissions_dao import permission_dao +from app.dao.service_permissions_dao import dao_fetch_service_permissions from app.utils import get_template_instance @@ -180,11 +181,10 @@ class ServiceSchema(BaseSchema): branding = field_for(models.Service, 'branding') dvla_organisation = field_for(models.Service, 'dvla_organisation') permissions = fields.Method("service_permissions") - + def service_permissions(self, service): permissions = [] - from app.dao.service_permissions_dao import dao_fetch_service_permissions perms = dao_fetch_service_permissions(service.id) for p in perms: permission = { @@ -216,9 +216,17 @@ class ServiceSchema(BaseSchema): @validates('permissions') def validate_permissions(self, value): - for v in [val.permission for val in value]: - if v not in models.SERVICE_PERMISSION_TYPES: - raise ValidationError("Invalid Service Permission: '{}'".format(v)) + permissions = [v.permission for v in value] + for p in permissions: + if p not in models.SERVICE_PERMISSION_TYPES: + raise ValidationError("Invalid Service Permission: '{}'".format(p)) + + if len(set(permissions)) != len(permissions): + duplicates = list(set([x for x in permissions if permissions.count(x) > 1])) + raise ValueError('Service Permission duplicated: {}'.format(duplicates)) + + if INTERNATIONAL_SMS_TYPE in permissions and SMS_TYPE not in permissions: + raise ValueError('International SMS must have SMS enabled') @pre_load() def format_permissions_for_data_model(self, in_data): diff --git a/app/service/rest.py b/app/service/rest.py index c170420f9..de1450e09 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -129,7 +129,10 @@ def update_service(service_id): current_data = dict(service_schema.dump(fetched_service).data.items()) current_data.update(request.get_json()) - update_dict = service_schema.load(current_data).data + try: + update_dict = service_schema.load(current_data).data + except ValueError as e: + raise InvalidRequest(str(e), status_code=400) dao_update_service(update_dict) if service_going_live: diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 239c833fd..223ffa033 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -541,6 +541,44 @@ def test_update_permissions_with_an_invalid_permission_will_raise_error(client, assert "Invalid Service Permission: '{}'".format(invalid_permission) in result['message']['permissions'] +def test_update_permissions_with_duplicate_permissions_will_raise_error(client, sample_service): + auth_header = create_authorization_header() + + data = { + 'permissions': [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, LETTER_TYPE] + } + + resp = client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + result = json.loads(resp.get_data(as_text=True)) + + assert resp.status_code == 400 + assert result['result'] == 'error' + assert "Service Permission duplicated: ['{}']".format(LETTER_TYPE) in result['message'] + + +def test_update_permissions_with_international_sms_without_sms_permissions_will_raise_error(client, sample_service): + auth_header = create_authorization_header() + + data = { + 'permissions': [EMAIL_TYPE, INTERNATIONAL_SMS_TYPE] + } + + resp = client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + result = json.loads(resp.get_data(as_text=True)) + + assert resp.status_code == 400 + assert result['result'] == 'error' + assert "International SMS must have SMS enabled" == result['message'] + + def test_update_service_flags_can_remove_service_permissions(client, sample_service): auth_header = create_authorization_header() initial_data = { From 234312ece05332e2ced6ddd4889440db7f82edf0 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 23 May 2017 13:42:46 +0100 Subject: [PATCH 29/42] Update service permissions to ensure state in sync --- app/dao/services_dao.py | 28 +++++++++++-- app/schemas.py | 29 ++++++++++++-- tests/app/conftest.py | 7 ++-- tests/app/service/test_rest.py | 73 ++++++++++++++-------------------- 4 files changed, 84 insertions(+), 53 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index f1c66126a..ab4e71b56 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -31,7 +31,9 @@ from app.models import ( TEMPLATE_TYPES, JobStatistics, SMS_TYPE, - EMAIL_TYPE + EMAIL_TYPE, + INTERNATIONAL_SMS_TYPE, + LETTER_TYPE ) from app.service.statistics import format_monthly_template_notification_stats from app.statsd_decorators import statsd @@ -136,10 +138,28 @@ def dao_create_service(service, user, service_id=None, service_permissions=[SMS_ service.active = True service.research_mode = False - for permission in service_permissions: - service_permission = ServicePermission(service_id=service.id, permission=permission) - service.permissions.append(service_permission) + def process_deprecated_service_permissions(): + for permission in service_permissions: + service_permission = ServicePermission(service_id=service.id, permission=permission) + service.permissions.append(service_permission) + if permission == INTERNATIONAL_SMS_TYPE: + service.can_send_international_sms = True + if permission == LETTER_TYPE: + service.can_send_letters = True + + def sync_flags(flag, notify_type): + if flag and notify_type not in service_permissions: + service_permission = ServicePermission(service_id=service.id, permission=notify_type) + service.permissions.append(service_permission) + elif flag is False and notify_type in service_permissions: + service_permission = ServicePermission(service_id=service.id, permission=notify_type) + service.permissions.remove(service_permission) + + sync_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE) + sync_flags(service.can_send_letters, LETTER_TYPE) + + process_deprecated_service_permissions() db.session.add(service) diff --git a/app/schemas.py b/app/schemas.py index dd9e0d0ae..d9d71c991 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -25,7 +25,7 @@ from notifications_utils.recipients import ( from app import ma from app import models -from app.models import ServicePermission, INTERNATIONAL_SMS_TYPE, SMS_TYPE +from app.models import ServicePermission, INTERNATIONAL_SMS_TYPE, SMS_TYPE, LETTER_TYPE from app.dao.permissions_dao import permission_dao from app.dao.service_permissions_dao import dao_fetch_service_permissions from app.utils import get_template_instance @@ -184,6 +184,7 @@ class ServiceSchema(BaseSchema): def service_permissions(self, service): permissions = [] + str_permissions = [] perms = dao_fetch_service_permissions(service.id) for p in perms: @@ -192,6 +193,28 @@ class ServiceSchema(BaseSchema): "permission": p.permission } permissions.append(permission) + str_permissions.append(p.permission) + + def process_deprecated_permission_flags(): + def sync_flags(flag, notify_type): + if flag and notify_type not in str_permissions: + permission = { + "service_id": service.id, + "permission": notify_type + } + permissions.append(permission) + elif flag is False and notify_type in str_permissions: + permission = { + "service_id": service.id, + "permission": notify_type + } + permissions.remove(permission) + + sync_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE) + sync_flags(service.can_send_letters, LETTER_TYPE) + + process_deprecated_permission_flags() + return permissions class Meta: @@ -232,7 +255,7 @@ class ServiceSchema(BaseSchema): def format_permissions_for_data_model(self, in_data): if isinstance(in_data, dict) and 'permissions' in in_data: permissions = [] - for p in in_data.get('permissions'): + for p in in_data['permissions']: permission = models.ServicePermission(service_id=in_data["id"], permission=p) permissions.append(permission) in_data['permissions'] = permissions @@ -240,7 +263,7 @@ class ServiceSchema(BaseSchema): @post_dump def format_permissions_as_string_array(self, in_data): if isinstance(in_data, dict) and 'permissions' in in_data: - in_data['permissions'] = [p.get('permission') for p in in_data.get('permissions')] + in_data['permissions'] = [p.get("permission") for p in in_data['permissions']] return in_data diff --git a/tests/app/conftest.py b/tests/app/conftest.py index b7ca092ba..a95228426 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -24,7 +24,7 @@ from app.models import ( NotificationStatistics, ServiceWhitelist, KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM, - MOBILE_TYPE, EMAIL_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED, ScheduledNotification) + MOBILE_TYPE, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED, ScheduledNotification) from app.dao.users_dao import (create_user_code, create_secret_code) from app.dao.services_dao import (dao_create_service, dao_add_user_to_service) from app.dao.templates_dao import dao_create_template @@ -124,7 +124,8 @@ def sample_service( restricted=False, limit=1000, email_from=None, - can_send_international_sms=False + can_send_international_sms=False, + permissions=[SMS_TYPE, EMAIL_TYPE] ): if user is None: user = create_user() @@ -142,7 +143,7 @@ def sample_service( service = Service.query.filter_by(name=service_name).first() if not service: service = Service(**data) - dao_create_service(service, user) + dao_create_service(service, user, service_permissions=permissions) else: if user not in service.users: dao_add_user_to_service(service, user) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 223ffa033..dbb00615a 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -21,6 +21,7 @@ from tests.app.conftest import ( sample_notification_with_job ) from app.models import ( + ServicePermission, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE ) @@ -157,7 +158,7 @@ def test_get_service_list_has_default_permissions(client, service_factory): assert response.status_code == 200 json_resp = json.loads(response.get_data(as_text=True)) assert len(json_resp['data']) == 3 - assert all([set(json['permissions']) & set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']]) + assert all([set(json['permissions']) == set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']]) def test_get_service_by_id_has_default_permissions(client, sample_service): @@ -490,18 +491,41 @@ def test_update_service_flags_will_add_service_permissions(client, sample_servic assert result['data']['research_mode'] is True assert result['data']['can_send_letters'] is True assert result['data']['can_send_international_sms'] is True - assert all(set(result['data']['permissions']) & set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE])) + assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE]) + + +def test_update_service_flags_will_remove_service_permissions(client, notify_db, notify_db_session): + auth_header = create_authorization_header() + + service = create_service(notify_db, notify_db_session, can_send_international_sms=True) + + assert service.can_send_international_sms is True + assert set([p.permission for p in service.permissions]) == set([SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE]) + + data = { + 'can_send_international_sms': False, + } + + resp = client.post( + '/service/{}'.format(service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + result = json.loads(resp.get_data(as_text=True)) + + assert resp.status_code == 200 + assert result['data']['can_send_international_sms'] is False + assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE]) def test_update_permissions_can_add_service_permissions(client, sample_service): - from app.models import ServicePermission auth_header = create_authorization_header() data = { 'research_mode': True, - 'can_send_letters': True, 'can_send_international_sms': True, - 'permissions': [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE] + 'can_send_letters': True, + 'permissions': [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE] } resp = client.post( @@ -515,7 +539,7 @@ def test_update_permissions_can_add_service_permissions(client, sample_service): assert result['data']['research_mode'] is True assert result['data']['can_send_letters'] is True assert result['data']['can_send_international_sms'] is True - assert all(set(result['data']['permissions']) & set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE])) + assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE]) def test_update_permissions_with_an_invalid_permission_will_raise_error(client, sample_service): @@ -523,9 +547,6 @@ def test_update_permissions_with_an_invalid_permission_will_raise_error(client, invalid_permission = 'invalid_permission' data = { - 'research_mode': True, - 'can_send_letters': True, - 'can_send_international_sms': True, 'permissions': [EMAIL_TYPE, SMS_TYPE, invalid_permission] } @@ -579,40 +600,6 @@ def test_update_permissions_with_international_sms_without_sms_permissions_will_ assert "International SMS must have SMS enabled" == result['message'] -def test_update_service_flags_can_remove_service_permissions(client, sample_service): - auth_header = create_authorization_header() - initial_data = { - 'research_mode': True, - 'can_send_letters': True, - 'can_send_international_sms': True, - 'permissions': [EMAIL_TYPE, SMS_TYPE] - } - - resp = client.post( - '/service/{}'.format(sample_service.id), - data=json.dumps(initial_data), - headers=[('Content-Type', 'application/json'), auth_header] - ) - - remove_permission_data = { - 'can_send_letters': True, - 'can_send_international_sms': False, - 'permissions': [EMAIL_TYPE] - } - - resp = client.post( - '/service/{}'.format(sample_service.id), - data=json.dumps(remove_permission_data), - headers=[('Content-Type', 'application/json'), auth_header] - ) - result = json.loads(resp.get_data(as_text=True)) - - assert resp.status_code == 200 - assert result['data']['can_send_letters'] is True - assert result['data']['can_send_international_sms'] is False - assert result['data']['permissions'] == [EMAIL_TYPE] - - def test_update_service_research_mode_throws_validation_error(notify_api, sample_service): with notify_api.test_request_context(): with notify_api.test_client() as client: From f7a18f77cf0100345d35809db802da89ef7caf27 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 23 May 2017 14:24:07 +0100 Subject: [PATCH 30/42] Update model to cascade permissions assoc proxy --- app/dao/services_dao.py | 10 ---------- app/models.py | 3 ++- tests/app/dao/test_services_dao.py | 10 ++++------ 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index ab4e71b56..6d0b22ef8 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -169,16 +169,6 @@ def dao_update_service(service): db.session.add(service) -@transactional -@version_class(Service) -def dao_remove_service_permission(service, permission): - for p in service.permissions: - if p.permission == permission: - service.permissions.remove(p) - - db.session.add(service) - - def dao_add_user_to_service(service, user, permissions=None): permissions = permissions or [] try: diff --git a/app/models.py b/app/models.py index ba667f599..8c1e22e79 100644 --- a/app/models.py +++ b/app/models.py @@ -241,7 +241,8 @@ class ServicePermission(db.Model): service = db.relationship("Service") created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False) - service_permission_types = db.relationship(Service, backref=db.backref("permissions")) + service_permission_types = db.relationship( + Service, backref=db.backref("permissions", cascade="all, delete-orphan")) def __repr__(self): return '<{} has service permission: {}>'.format(self.service_id, self.permission) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 0c78a7940..aac063f9b 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -11,7 +11,6 @@ from app.dao.services_dao import ( dao_create_service, dao_add_user_to_service, dao_remove_user_from_service, - dao_remove_service_permission as dao_services_remove_service_permission, dao_fetch_all_services, dao_fetch_service_by_id, dao_fetch_all_services_by_user, @@ -46,10 +45,7 @@ from app.models import ( InvitedUser, Service, ServicePermission, -<<<<<<< HEAD ServicePermissionTypes, -======= ->>>>>>> Updated service DAO and API end points BRANDING_GOVUK, DVLA_ORG_HM_GOVERNMENT, KEY_TYPE_NORMAL, @@ -298,7 +294,7 @@ def test_remove_service_does_not_remove_service_permission_types(sample_service) services = dao_fetch_all_services() assert len(services) == 0 - assert set([p.name for p in ServicePermissionTypes.query.all()]) & set(SERVICE_PERMISSION_TYPES) + assert set([p.name for p in ServicePermissionTypes.query.all()]) == set(SERVICE_PERMISSION_TYPES) def test_create_service_by_id_adding_and_removing_letter_returns_service_without_letter(service_factory): @@ -397,7 +393,9 @@ def test_update_service_permission_creates_a_history_record_with_current_data(sa assert service_from_db.version == 2 assert LETTER_TYPE in [p.permission for p in service_from_db.permissions] - dao_services_remove_service_permission(service, permission='sms') + permission = [p for p in service.permissions if p.permission == 'sms'][0] + service.permissions.remove(permission) + dao_update_service(service) assert Service.query.count() == 1 assert Service.get_history_model().query.count() == 3 From 8488895612c9ebc6db49fb81e0dddadb139f0452 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 23 May 2017 17:01:27 +0100 Subject: [PATCH 31/42] Refactored tests --- tests/app/dao/test_services_dao.py | 57 +++++++++++++++++++++++++----- tests/app/service/test_rest.py | 24 +++++++++++-- 2 files changed, 71 insertions(+), 10 deletions(-) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index aac063f9b..b7b3176dd 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -258,15 +258,15 @@ def test_create_service_returns_service_with_default_permissions(service_factory service = dao_fetch_service_by_id(service.id) assert len(service.permissions) == 2 - assert all(p.permission in [SMS_TYPE, EMAIL_TYPE] for p in service.permissions) + assert set([SMS_TYPE, EMAIL_TYPE]) == set(p.permission for p in service.permissions) # This test is only for backward compatibility and will be removed -# when the 'can_use' columns are dropped from the Service data model +# when the deprecated 'can_use' columns are not used in the Service data model @pytest.mark.parametrize("permission_to_add, can_send_letters, can_send_international_sms", [(LETTER_TYPE, True, False), (INTERNATIONAL_SMS_TYPE, False, True)]) -def test_create_service_by_id_adding_service_permission_returns_service_with_permissions_set( +def test_create_service_by_id_adding_service_permission_returns_service_with_flags_and_permissions_set( service_factory, permission_to_add, can_send_letters, can_send_international_sms): service = service_factory.get('testing', email_from='testing') @@ -275,18 +275,59 @@ def test_create_service_by_id_adding_service_permission_returns_service_with_per service = dao_fetch_service_by_id(service.id) assert len(service.permissions) == 3 - assert all(p.permission in [SMS_TYPE, EMAIL_TYPE, permission_to_add] for p in service.permissions) + assert set([SMS_TYPE, EMAIL_TYPE, permission_to_add]) == set(p.permission for p in service.permissions) assert service.can_send_letters == can_send_letters assert service.can_send_international_sms == can_send_international_sms -def test_remove_permission_from_service_by_id_returns_service_with_correct_permissions(service_factory): +# This test is only for backward compatibility and will be removed +# when the deprecated 'can_use' columns are not used in the Service data model +@pytest.mark.parametrize("permission_to_remove, can_send_letters, can_send_international_sms", + [(LETTER_TYPE, False, True), + (INTERNATIONAL_SMS_TYPE, True, False)]) +def test_create_service_by_id_removing_service_permission_returns_service_with_flags_and_permissions_set( + service_factory, permission_to_remove, can_send_letters, can_send_international_sms): service = service_factory.get('testing', email_from='testing') - dao_remove_service_permission(service_id=service.id, permission=SMS_TYPE) + + dao_add_service_permission(service_id=service.id, permission=LETTER_TYPE) + dao_add_service_permission(service_id=service.id, permission=INTERNATIONAL_SMS_TYPE) + service = dao_fetch_service_by_id(service.id) + service.set_permissions() + assert len(service.permissions) == 4 + assert service.can_send_letters + assert service.can_send_international_sms + + dao_remove_service_permission(service_id=service.id, permission=permission_to_remove) + service.set_permissions() service = dao_fetch_service_by_id(service.id) + expected_permissions = [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE] + expected_permissions.remove(permission_to_remove) + + assert len(service.permissions) == 3 + assert set(expected_permissions) == set(p.permission for p in service.permissions) + assert service.can_send_letters == can_send_letters + assert service.can_send_international_sms == can_send_international_sms + + +@pytest.mark.parametrize("permission_to_remove, permission_remaining", + [(SMS_TYPE, EMAIL_TYPE), + (EMAIL_TYPE, SMS_TYPE)]) +def test_remove_permission_from_service_by_id_returns_service_with_correct_permissions( + sample_service, permission_to_remove, permission_remaining): + dao_remove_service_permission(service_id=sample_service.id, permission=permission_to_remove) + + service = dao_fetch_service_by_id(sample_service.id) assert len(service.permissions) == 1 - assert service.permissions[0].permission == EMAIL_TYPE + assert service.permissions[0].permission == permission_remaining + + +def test_removing_all_permission_returns_service_with_no_permissions(sample_service): + dao_remove_service_permission(service_id=sample_service.id, permission=SMS_TYPE) + dao_remove_service_permission(service_id=sample_service.id, permission=EMAIL_TYPE) + + service = dao_fetch_service_by_id(sample_service.id) + assert len(service.permissions) == 0 def test_remove_service_does_not_remove_service_permission_types(sample_service): @@ -294,7 +335,7 @@ def test_remove_service_does_not_remove_service_permission_types(sample_service) services = dao_fetch_all_services() assert len(services) == 0 - assert set([p.name for p in ServicePermissionTypes.query.all()]) == set(SERVICE_PERMISSION_TYPES) + assert set(p.name for p in ServicePermissionTypes.query.all()) == set(SERVICE_PERMISSION_TYPES) def test_create_service_by_id_adding_and_removing_letter_returns_service_without_letter(service_factory): diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index dbb00615a..be0948bed 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -22,7 +22,8 @@ from tests.app.conftest import ( ) from app.models import ( ServicePermission, - KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE + KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, + EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE ) from tests.app.db import create_user @@ -161,7 +162,7 @@ def test_get_service_list_has_default_permissions(client, service_factory): assert all([set(json['permissions']) == set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']]) -def test_get_service_by_id_has_default_permissions(client, sample_service): +def test_get_service_by_id_has_default_service_permissions(client, sample_service): auth_header = create_authorization_header() resp = client.get( '/service/{}'.format(sample_service.id), @@ -542,6 +543,24 @@ def test_update_permissions_can_add_service_permissions(client, sample_service): assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE]) +def test_add_inbound_permissions_will_add_service_permissions(client, sample_service): + auth_header = create_authorization_header() + + data = { + 'permissions': [EMAIL_TYPE, SMS_TYPE, INBOUND_SMS_TYPE] + } + + resp = client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + result = json.loads(resp.get_data(as_text=True)) + + assert resp.status_code == 200 + assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, INBOUND_SMS_TYPE]) + + def test_update_permissions_with_an_invalid_permission_will_raise_error(client, sample_service): auth_header = create_authorization_header() invalid_permission = 'invalid_permission' @@ -585,6 +604,7 @@ def test_update_permissions_with_international_sms_without_sms_permissions_will_ auth_header = create_authorization_header() data = { + 'can_send_international_sms': True, 'permissions': [EMAIL_TYPE, INTERNATIONAL_SMS_TYPE] } From 54d85fd8df03bbff730b7c9603c430c1b68a7659 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 24 May 2017 10:45:05 +0100 Subject: [PATCH 32/42] Add tests for services with no permissions --- app/schemas.py | 48 ++++++++++++++----- app/service/rest.py | 1 + tests/app/service/test_rest.py | 88 +++++++++++++++++++--------------- 3 files changed, 87 insertions(+), 50 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index d9d71c991..2045f9c31 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -25,7 +25,7 @@ from notifications_utils.recipients import ( from app import ma from app import models -from app.models import ServicePermission, INTERNATIONAL_SMS_TYPE, SMS_TYPE, LETTER_TYPE +from app.models import ServicePermission, INTERNATIONAL_SMS_TYPE, SMS_TYPE, LETTER_TYPE, EMAIL_TYPE from app.dao.permissions_dao import permission_dao from app.dao.service_permissions_dao import dao_fetch_service_permissions from app.utils import get_template_instance @@ -181,6 +181,7 @@ class ServiceSchema(BaseSchema): branding = field_for(models.Service, 'branding') dvla_organisation = field_for(models.Service, 'dvla_organisation') permissions = fields.Method("service_permissions") + override_flag = False def service_permissions(self, service): permissions = [] @@ -195,8 +196,8 @@ class ServiceSchema(BaseSchema): permissions.append(permission) str_permissions.append(p.permission) - def process_deprecated_permission_flags(): - def sync_flags(flag, notify_type): + def deprecate_convert_flags_to_permissions(): + def convert_flags(flag, notify_type): if flag and notify_type not in str_permissions: permission = { "service_id": service.id, @@ -210,10 +211,10 @@ class ServiceSchema(BaseSchema): } permissions.remove(permission) - sync_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE) - sync_flags(service.can_send_letters, LETTER_TYPE) + convert_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE) + convert_flags(service.can_send_letters, LETTER_TYPE) - process_deprecated_permission_flags() + deprecate_convert_flags_to_permissions() return permissions @@ -248,24 +249,47 @@ class ServiceSchema(BaseSchema): duplicates = list(set([x for x in permissions if permissions.count(x) > 1])) raise ValueError('Service Permission duplicated: {}'.format(duplicates)) - if INTERNATIONAL_SMS_TYPE in permissions and SMS_TYPE not in permissions: - raise ValueError('International SMS must have SMS enabled') - @pre_load() - def format_permissions_for_data_model(self, in_data): + def format_for_data_model(self, in_data): if isinstance(in_data, dict) and 'permissions' in in_data: + str_permissions = in_data['permissions'] permissions = [] for p in in_data['permissions']: - permission = models.ServicePermission(service_id=in_data["id"], permission=p) + permission = ServicePermission(service_id=in_data["id"], permission=p) permissions.append(permission) in_data['permissions'] = permissions + def deprecate_override_flags(): + in_data['can_send_letters'] = LETTER_TYPE in [p.permission for p in permissions] + in_data['can_send_international_sms'] = INTERNATIONAL_SMS_TYPE in [p.permission for p in permissions] + + def deprecate_convert_flags_to_permissions(): + def convert_flag(flag, notify_type): + if flag and notify_type not in str_permissions: + permission = ServicePermission(service_id=in_data['id'], permission=notify_type) + permissions.append(permission) + elif flag is False and notify_type in str_permissions: + for p in permissions: + if p.permission == notify_type: + permissions.remove(p) + + convert_flag(in_data["can_send_international_sms"], INTERNATIONAL_SMS_TYPE) + convert_flag(in_data["can_send_letters"], LETTER_TYPE) + + if self.override_flag: + deprecate_override_flags() + else: + deprecate_convert_flags_to_permissions() + @post_dump - def format_permissions_as_string_array(self, in_data): + def format_as_string_array(self, in_data): if isinstance(in_data, dict) and 'permissions' in in_data: in_data['permissions'] = [p.get("permission") for p in in_data['permissions']] return in_data + def set_override_flag(self, flag): + self.override_flag = flag + class DetailedServiceSchema(BaseSchema): statistics = fields.Dict() diff --git a/app/service/rest.py b/app/service/rest.py index de1450e09..b3977f2b8 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -128,6 +128,7 @@ def update_service(service_id): service_going_live = fetched_service.restricted and not request.get_json().get('restricted', True) current_data = dict(service_schema.dump(fetched_service).data.items()) + service_schema.set_override_flag(request.get_json().get('permissions') is not None) current_data.update(request.get_json()) try: update_dict = service_schema.load(current_data).data diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index be0948bed..4977223eb 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -22,7 +22,7 @@ from tests.app.conftest import ( ) from app.models import ( ServicePermission, - KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, + KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE ) @@ -473,26 +473,29 @@ def test_update_service_flags(client, sample_service): assert result['data']['can_send_international_sms'] is True -def test_update_service_flags_will_add_service_permissions(client, sample_service): +@pytest.fixture(scope='function') +def service_with_no_permissions(notify_db, notify_db_session): + return create_service(notify_db, notify_db_session, permissions=[]) + + +def test_update_service_flags_with_service_without_default_service_permissions(client, service_with_no_permissions): auth_header = create_authorization_header() data = { - 'research_mode': True, 'can_send_letters': True, 'can_send_international_sms': True, } resp = client.post( - '/service/{}'.format(sample_service.id), + '/service/{}'.format(service_with_no_permissions.id), data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header] ) result = json.loads(resp.get_data(as_text=True)) assert resp.status_code == 200 - assert result['data']['research_mode'] is True assert result['data']['can_send_letters'] is True assert result['data']['can_send_international_sms'] is True - assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE]) + assert set(result['data']['permissions']) == set([LETTER_TYPE, INTERNATIONAL_SMS_TYPE]) def test_update_service_flags_will_remove_service_permissions(client, notify_db, notify_db_session): @@ -504,7 +507,7 @@ def test_update_service_flags_will_remove_service_permissions(client, notify_db, assert set([p.permission for p in service.permissions]) == set([SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE]) data = { - 'can_send_international_sms': False, + 'can_send_international_sms': False } resp = client.post( @@ -519,35 +522,31 @@ def test_update_service_flags_will_remove_service_permissions(client, notify_db, assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE]) -def test_update_permissions_can_add_service_permissions(client, sample_service): +def test_update_permissions_will_override_permission_flags(client, service_with_no_permissions): auth_header = create_authorization_header() data = { - 'research_mode': True, - 'can_send_international_sms': True, - 'can_send_letters': True, - 'permissions': [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE] + 'permissions': [LETTER_TYPE, INTERNATIONAL_SMS_TYPE] } resp = client.post( - '/service/{}'.format(sample_service.id), + '/service/{}'.format(service_with_no_permissions.id), data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header] ) result = json.loads(resp.get_data(as_text=True)) assert resp.status_code == 200 - assert result['data']['research_mode'] is True assert result['data']['can_send_letters'] is True assert result['data']['can_send_international_sms'] is True - assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE]) + assert set(result['data']['permissions']) == set([LETTER_TYPE, INTERNATIONAL_SMS_TYPE]) -def test_add_inbound_permissions_will_add_service_permissions(client, sample_service): +def test_update_service_permissions_will_add_service_permissions(client, sample_service): auth_header = create_authorization_header() data = { - 'permissions': [EMAIL_TYPE, SMS_TYPE, INBOUND_SMS_TYPE] + 'permissions': [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE] } resp = client.post( @@ -558,7 +557,40 @@ def test_add_inbound_permissions_will_add_service_permissions(client, sample_ser result = json.loads(resp.get_data(as_text=True)) assert resp.status_code == 200 - assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, INBOUND_SMS_TYPE]) + assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE]) + + +@pytest.mark.parametrize( + 'permission_to_add', + [ + (EMAIL_TYPE), + (SMS_TYPE), + (INTERNATIONAL_SMS_TYPE), + (LETTER_TYPE), + (INBOUND_SMS_TYPE), + ] +) +def test_add_service_permission_will_add_permission(client, service_with_no_permissions, permission_to_add): + auth_header = create_authorization_header() + + data = { + 'permissions': [permission_to_add] + } + + resp = client.post( + '/service/{}'.format(service_with_no_permissions.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + resp = client.get( + '/service/{}'.format(service_with_no_permissions.id), + headers=[auth_header] + ) + result = json.loads(resp.get_data(as_text=True)) + + assert resp.status_code == 200 + assert result['data']['permissions'] == [permission_to_add] def test_update_permissions_with_an_invalid_permission_will_raise_error(client, sample_service): @@ -600,26 +632,6 @@ def test_update_permissions_with_duplicate_permissions_will_raise_error(client, assert "Service Permission duplicated: ['{}']".format(LETTER_TYPE) in result['message'] -def test_update_permissions_with_international_sms_without_sms_permissions_will_raise_error(client, sample_service): - auth_header = create_authorization_header() - - data = { - 'can_send_international_sms': True, - 'permissions': [EMAIL_TYPE, INTERNATIONAL_SMS_TYPE] - } - - resp = client.post( - '/service/{}'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header] - ) - result = json.loads(resp.get_data(as_text=True)) - - assert resp.status_code == 400 - assert result['result'] == 'error' - assert "International SMS must have SMS enabled" == result['message'] - - def test_update_service_research_mode_throws_validation_error(notify_api, sample_service): with notify_api.test_request_context(): with notify_api.test_client() as client: From 1375bbe400a82c3eb913c3859b7084dbb45bca9c Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 25 May 2017 17:45:15 +0100 Subject: [PATCH 33/42] Refactor schema to improve error response --- app/dao/services_dao.py | 10 +++++----- app/schemas.py | 8 ++++---- app/service/rest.py | 5 +---- tests/app/service/test_rest.py | 4 ++-- 4 files changed, 12 insertions(+), 15 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 6d0b22ef8..412723830 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -138,7 +138,7 @@ def dao_create_service(service, user, service_id=None, service_permissions=[SMS_ service.active = True service.research_mode = False - def process_deprecated_service_permissions(): + def deprecate_process_service_permissions(): for permission in service_permissions: service_permission = ServicePermission(service_id=service.id, permission=permission) service.permissions.append(service_permission) @@ -148,7 +148,7 @@ def dao_create_service(service, user, service_id=None, service_permissions=[SMS_ if permission == LETTER_TYPE: service.can_send_letters = True - def sync_flags(flag, notify_type): + def convert_flags(flag, notify_type): if flag and notify_type not in service_permissions: service_permission = ServicePermission(service_id=service.id, permission=notify_type) service.permissions.append(service_permission) @@ -156,10 +156,10 @@ def dao_create_service(service, user, service_id=None, service_permissions=[SMS_ service_permission = ServicePermission(service_id=service.id, permission=notify_type) service.permissions.remove(service_permission) - sync_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE) - sync_flags(service.can_send_letters, LETTER_TYPE) + convert_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE) + convert_flags(service.can_send_letters, LETTER_TYPE) - process_deprecated_service_permissions() + deprecate_process_service_permissions() db.session.add(service) diff --git a/app/schemas.py b/app/schemas.py index 2045f9c31..ff4f83a95 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -247,7 +247,7 @@ class ServiceSchema(BaseSchema): if len(set(permissions)) != len(permissions): duplicates = list(set([x for x in permissions if permissions.count(x) > 1])) - raise ValueError('Service Permission duplicated: {}'.format(duplicates)) + raise ValidationError('Duplicate Service Permission: {}'.format(duplicates)) @pre_load() def format_for_data_model(self, in_data): @@ -264,7 +264,7 @@ class ServiceSchema(BaseSchema): in_data['can_send_international_sms'] = INTERNATIONAL_SMS_TYPE in [p.permission for p in permissions] def deprecate_convert_flags_to_permissions(): - def convert_flag(flag, notify_type): + def convert_flags(flag, notify_type): if flag and notify_type not in str_permissions: permission = ServicePermission(service_id=in_data['id'], permission=notify_type) permissions.append(permission) @@ -273,8 +273,8 @@ class ServiceSchema(BaseSchema): if p.permission == notify_type: permissions.remove(p) - convert_flag(in_data["can_send_international_sms"], INTERNATIONAL_SMS_TYPE) - convert_flag(in_data["can_send_letters"], LETTER_TYPE) + convert_flags(in_data["can_send_international_sms"], INTERNATIONAL_SMS_TYPE) + convert_flags(in_data["can_send_letters"], LETTER_TYPE) if self.override_flag: deprecate_override_flags() diff --git a/app/service/rest.py b/app/service/rest.py index b3977f2b8..122a16c8c 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -130,10 +130,7 @@ def update_service(service_id): current_data = dict(service_schema.dump(fetched_service).data.items()) service_schema.set_override_flag(request.get_json().get('permissions') is not None) current_data.update(request.get_json()) - try: - update_dict = service_schema.load(current_data).data - except ValueError as e: - raise InvalidRequest(str(e), status_code=400) + update_dict = service_schema.load(current_data).data dao_update_service(update_dict) if service_going_live: diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 4977223eb..11d50f899 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -21,7 +21,7 @@ from tests.app.conftest import ( sample_notification_with_job ) from app.models import ( - ServicePermission, + Service, ServicePermission, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE ) @@ -629,7 +629,7 @@ def test_update_permissions_with_duplicate_permissions_will_raise_error(client, assert resp.status_code == 400 assert result['result'] == 'error' - assert "Service Permission duplicated: ['{}']".format(LETTER_TYPE) in result['message'] + assert "Duplicate Service Permission: ['{}']".format(LETTER_TYPE) in result['message']['permissions'] def test_update_service_research_mode_throws_validation_error(notify_api, sample_service): From cbc92a6173eff5db92639b287218794b81cee2c3 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 23 May 2017 14:47:55 +0100 Subject: [PATCH 34/42] Store the normalised number on the notification --- app/celery/tasks.py | 27 +++++++------- app/notifications/process_notifications.py | 1 + .../test_process_notification.py | 35 +++++++++++++++++++ 3 files changed, 50 insertions(+), 13 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index cce6b05f0..5cd860fe3 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -165,19 +165,20 @@ def send_sms(self, return try: - saved_notification = persist_notification(template_id=notification['template'], - template_version=notification['template_version'], - recipient=notification['to'], - service=service, - personalisation=notification.get('personalisation'), - notification_type=SMS_TYPE, - api_key_id=api_key_id, - key_type=key_type, - created_at=created_at, - job_id=notification.get('job', None), - job_row_number=notification.get('row_number', None), - notification_id=notification_id - ) + saved_notification = persist_notification( + template_id=notification['template'], + template_version=notification['template_version'], + recipient=notification['to'], + service=service, + personalisation=notification.get('personalisation'), + notification_type=SMS_TYPE, + api_key_id=api_key_id, + key_type=key_type, + created_at=created_at, + job_id=notification.get('job', None), + job_row_number=notification.get('row_number', None), + notification_id=notification_id + ) provider_tasks.deliver_sms.apply_async( [str(saved_notification.id)], diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 20550be7a..492e62d19 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -72,6 +72,7 @@ def persist_notification( if notification_type == SMS_TYPE: formatted_recipient = validate_and_format_phone_number(recipient, international=True) recipient_info = get_international_phone_info(formatted_recipient) + notification.normalised_to = formatted_recipient notification.international = recipient_info.international notification.phone_prefix = recipient_info.country_prefix notification.rate_multiplier = recipient_info.billable_units diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 6df3862a3..100adf6fa 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -348,3 +348,38 @@ def test_persist_scheduled_notification(sample_notification): assert len(scheduled_notification) == 1 assert scheduled_notification[0].notification_id == sample_notification.id assert scheduled_notification[0].scheduled_for == datetime.datetime(2017, 5, 12, 13, 15) + + +@pytest.mark.parametrize('recipient, expected_recipient_normalised', [ + ('7900900123', '447900900123'), + ('+447900 900 123', '447900900123'), + (' 07700900222', '447700900222'), + ('07700900222', '447700900222'), + (' 73122345678', '73122345678'), + ('360623400400', '360623400400'), + ('-077-00900222-', '447700900222'), + ('(360623(400400)', '360623400400') + +]) +def test_persist_sms_notification_stores_normalised_number( + sample_job, + sample_api_key, + mocker, + recipient, + expected_recipient_normalised +): + persist_notification( + template_id=sample_job.template.id, + template_version=sample_job.template.version, + recipient=recipient, + service=sample_job.service, + personalisation=None, + notification_type='sms', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + job_id=sample_job.id, + ) + persisted_notification = Notification.query.all()[0] + + assert persisted_notification.to == recipient + assert persisted_notification.normalised_to == expected_recipient_normalised From 6c4377bd444ed5c8792c105b6085db3b00fe6ce9 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 23 May 2017 15:45:11 +0100 Subject: [PATCH 35/42] Persist normalised email --- app/notifications/process_notifications.py | 5 +++- .../test_process_notification.py | 29 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 492e62d19..a6d1137ce 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -4,7 +4,8 @@ from flask import current_app from notifications_utils.recipients import ( get_international_phone_info, - validate_and_format_phone_number + validate_and_format_phone_number, + format_email_address ) from app import redis_store @@ -76,6 +77,8 @@ def persist_notification( notification.international = recipient_info.international notification.phone_prefix = recipient_info.country_prefix notification.rate_multiplier = recipient_info.billable_units + elif notification_type == EMAIL_TYPE: + notification.normalised_to = format_email_address(notification.to) # if simulated create a Notification model to return but do not persist the Notification to the dB if not simulated: diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 100adf6fa..2f66ef457 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -383,3 +383,32 @@ def test_persist_sms_notification_stores_normalised_number( assert persisted_notification.to == recipient assert persisted_notification.normalised_to == expected_recipient_normalised + + +@pytest.mark.parametrize('recipient, expected_recipient_normalised', [ + ('FOO@bar.com', 'foo@bar.com'), + ('BAR@foo.com', 'bar@foo.com') + +]) +def test_persist_email_notification_stores_normalised_email( + sample_job, + sample_api_key, + mocker, + recipient, + expected_recipient_normalised +): + persist_notification( + template_id=sample_job.template.id, + template_version=sample_job.template.version, + recipient=recipient, + service=sample_job.service, + personalisation=None, + notification_type='email', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + job_id=sample_job.id, + ) + persisted_notification = Notification.query.all()[0] + + assert persisted_notification.to == recipient + assert persisted_notification.normalised_to == expected_recipient_normalised From cfe08a4d8b6a18a8348b25aed9e3003990ecc9bd Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 23 May 2017 14:47:55 +0100 Subject: [PATCH 36/42] Store the normalised number on the notification --- tests/app/notifications/test_process_notification.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 2f66ef457..7cbaac8fd 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -383,6 +383,7 @@ def test_persist_sms_notification_stores_normalised_number( assert persisted_notification.to == recipient assert persisted_notification.normalised_to == expected_recipient_normalised +<<<<<<< HEAD @pytest.mark.parametrize('recipient, expected_recipient_normalised', [ @@ -412,3 +413,5 @@ def test_persist_email_notification_stores_normalised_email( assert persisted_notification.to == recipient assert persisted_notification.normalised_to == expected_recipient_normalised +======= +>>>>>>> Store the normalised number on the notification From 78c10b7d306be23681d178a20750080cf3148f9d Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 24 May 2017 14:24:57 +0100 Subject: [PATCH 37/42] Search notification against normalised recipient with filter for status --- app/dao/notifications_dao.py | 24 +++- tests/app/dao/test_notification_dao.py | 108 +++++++++++++++--- tests/app/db.py | 21 +++- .../test_process_notification.py | 3 - 4 files changed, 130 insertions(+), 26 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d1b78c27a..7ebf9170c 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -5,6 +5,12 @@ from datetime import ( date) from flask import current_app + +from notifications_utils.recipients import ( + validate_and_format_phone_number, + validate_and_format_email_address, + InvalidPhoneError +) from werkzeug.datastructures import MultiDict from sqlalchemy import (desc, func, or_, and_, asc) from sqlalchemy.orm import joinedload @@ -467,10 +473,22 @@ def dao_update_notifications_sent_to_dvla(job_id, provider): @statsd(namespace="dao") -def dao_get_notifications_by_to_field(service_id, search_term): - return Notification.query.filter( +def dao_get_notifications_by_to_field(service_id, search_term, statuses=None): + try: + normalised = validate_and_format_phone_number(search_term) + except InvalidPhoneError: + normalised = validate_and_format_email_address(search_term) + + filters = [ Notification.service_id == service_id, - func.replace(func.lower(Notification.to), " ", "") == search_term.lower().replace(" ", "")).all() + Notification.normalised_to == normalised + ] + + if statuses: + filters.append(Notification.status.in_(statuses)) + + results = db.session.query(Notification).filter(*filters).all() + return results @statsd(namespace="dao") diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 38705ed42..a6b69cd24 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1746,33 +1746,53 @@ def test_dao_update_notifications_sent_to_dvla_does_update_history_if_test_key( def test_dao_get_notifications_by_to_field(sample_template): - notification1 = create_notification(template=sample_template, to_field='+447700900855') - notification2 = create_notification(template=sample_template, to_field='jack@gmail.com') - notification3 = create_notification(template=sample_template, to_field='jane@gmail.com') + notification1 = create_notification( + template=sample_template, to_field='+447700900855', normalised_to='447700900855' + ) + create_notification( + template=sample_template, to_field='jack@gmail.com', normalised_to='jack@gmail.com' + ) + create_notification( + template=sample_template, to_field='jane@gmail.com', normalised_to='jane@gmail.com' + ) results = dao_get_notifications_by_to_field(notification1.service_id, "+447700900855") + assert len(results) == 1 - assert results[0].id == notification1.id + assert notification1.id == results[0].id def test_dao_get_notifications_by_to_field_search_is_not_case_sensitive(sample_template): - notification1 = create_notification(template=sample_template, to_field='+447700900855') - notification2 = create_notification(template=sample_template, to_field='jack@gmail.com') - notification3 = create_notification(template=sample_template, to_field='jane@gmail.com') - results = dao_get_notifications_by_to_field(notification1.service_id, 'JACK@gmail.com') + notification = create_notification( + template=sample_template, to_field='jack@gmail.com', normalised_to='jack@gmail.com' + ) + results = dao_get_notifications_by_to_field(notification.service_id, 'JACK@gmail.com') + notification_ids = [notification.id for notification in results] + assert len(results) == 1 - assert results[0].id == notification2.id + assert notification.id in notification_ids def test_dao_get_notifications_by_to_field_search_ignores_spaces(sample_template): - notification1 = create_notification(template=sample_template, to_field='+447700900855') - notification2 = create_notification(template=sample_template, to_field='+44 77 00900 855') - notification3 = create_notification(template=sample_template, to_field=' +4477009 00 855 ') - notification4 = create_notification(template=sample_template, to_field='jack@gmail.com') + notification1 = create_notification( + template=sample_template, to_field='+447700900855', normalised_to='447700900855' + ) + notification2 = create_notification( + template=sample_template, to_field='+44 77 00900 855', normalised_to='447700900855' + ) + notification3 = create_notification( + template=sample_template, to_field=' +4477009 00 855 ', normalised_to='447700900855' + ) + create_notification( + template=sample_template, to_field='jaCK@gmail.com', normalised_to='jack@gmail.com' + ) + results = dao_get_notifications_by_to_field(notification1.service_id, '+447700900855') + notification_ids = [notification.id for notification in results] + assert len(results) == 3 - assert notification1.id in [r.id for r in results] - assert notification2.id in [r.id for r in results] - assert notification3.id in [r.id for r in results] + assert notification1.id in notification_ids + assert notification2.id in notification_ids + assert notification3.id in notification_ids def test_dao_created_scheduled_notification(sample_notification): @@ -1813,3 +1833,59 @@ def test_set_scheduled_notification_to_processed(notify_db, notify_db_session, s set_scheduled_notification_to_processed(notification_1.id) scheduled_notifications = dao_get_scheduled_notifications() assert not scheduled_notifications + + +def test_dao_get_notifications_by_to_field_filters_status(sample_template): + notification = create_notification( + template=sample_template, to_field='+447700900855', + normalised_to='447700900855', status='delivered' + ) + create_notification( + template=sample_template, to_field='+447700900855', + normalised_to='447700900855', status='temporary-failure' + ) + + notifications = dao_get_notifications_by_to_field(notification.service_id, "+447700900855", statuses=['delivered']) + + assert len(notifications) == 1 + assert notification.id == notifications[0].id + + +def test_dao_get_notifications_by_to_field_filters_multiple_statuses(sample_template): + notification1 = create_notification( + template=sample_template, to_field='+447700900855', + normalised_to='447700900855', status='delivered' + ) + notification2 = create_notification( + template=sample_template, to_field='+447700900855', + normalised_to='447700900855', status='sending' + ) + + notifications = dao_get_notifications_by_to_field( + notification1.service_id, "+447700900855", statuses=['delivered', 'sending'] + ) + notification_ids = [notification.id for notification in notifications] + + assert len(notifications) == 2 + assert notification1.id in notification_ids + assert notification2.id in notification_ids + + +def test_dao_get_notifications_by_to_field_returns_all_if_no_status_filter(sample_template): + notification1 = create_notification( + template=sample_template, to_field='+447700900855', + normalised_to='447700900855', status='delivered' + ) + notification2 = create_notification( + template=sample_template, to_field='+447700900855', + normalised_to='447700900855', status='temporary-failure' + ) + + notifications = dao_get_notifications_by_to_field( + notification1.service_id, "+447700900855" + ) + notification_ids = [notification.id for notification in notifications] + + assert len(notifications) == 2 + assert notification1.id in notification_ids + assert notification2.id in notification_ids diff --git a/tests/app/db.py b/tests/app/db.py index afb35e252..2f637cf1b 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,9 +1,20 @@ from datetime import datetime import uuid + from app.dao.jobs_dao import dao_create_job -from app.models import (Service, User, Template, Notification, EMAIL_TYPE, LETTER_TYPE, - SMS_TYPE, KEY_TYPE_NORMAL, Job, ServicePermission, ScheduledNotification) +from app.models import ( + Service, + User, + Template, + Notification, + ScheduledNotification, + ServicePermission, + Job, + EMAIL_TYPE, + SMS_TYPE, + KEY_TYPE_NORMAL, +) from app.dao.users_dao import save_model_user from app.dao.notifications_dao import dao_create_notification, dao_created_scheduled_notification from app.dao.templates_dao import dao_create_template @@ -81,7 +92,8 @@ def create_notification( rate_multiplier=None, international=False, phone_prefix=None, - scheduled_for=None + scheduled_for=None, + normalised_to=None ): if created_at is None: created_at = datetime.utcnow() @@ -115,7 +127,8 @@ def create_notification( 'job_row_number': job_row_number, 'rate_multiplier': rate_multiplier, 'international': international, - 'phone_prefix': phone_prefix + 'phone_prefix': phone_prefix, + 'normalised_to': normalised_to } notification = Notification(**data) dao_create_notification(notification) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 7cbaac8fd..2f66ef457 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -383,7 +383,6 @@ def test_persist_sms_notification_stores_normalised_number( assert persisted_notification.to == recipient assert persisted_notification.normalised_to == expected_recipient_normalised -<<<<<<< HEAD @pytest.mark.parametrize('recipient, expected_recipient_normalised', [ @@ -413,5 +412,3 @@ def test_persist_email_notification_stores_normalised_email( assert persisted_notification.to == recipient assert persisted_notification.normalised_to == expected_recipient_normalised -======= ->>>>>>> Store the normalised number on the notification From 77b82305f4563e59c3097386f8d97422faffd823 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 24 May 2017 14:25:38 +0100 Subject: [PATCH 38/42] Search normalised in get_notifications_for_service: * Use dao method to search against normalised(recipient) * Add filter to accept one or more statuses --- app/service/rest.py | 12 ++-- tests/app/conftest.py | 6 +- tests/app/service/test_rest.py | 124 ++++++++++++++++++++++++--------- 3 files changed, 103 insertions(+), 39 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 3270b96c2..b0de24869 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -265,8 +265,8 @@ def get_service_history(service_id): @service_blueprint.route('//notifications', methods=['GET']) def get_all_notifications_for_service(service_id): data = notifications_filter_schema.load(request.args).data - if data.get("to", None): - return search_for_notification_by_to_field(service_id, request.query_string.decode()) + if data.get('to'): + return search_for_notification_by_to_field(service_id, data['to'], statuses=data.get('status')) 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') @@ -296,11 +296,11 @@ def get_all_notifications_for_service(service_id): ), 200 -def search_for_notification_by_to_field(service_id, search_term): - search_term = search_term.replace('to=', '') - results = notifications_dao.dao_get_notifications_by_to_field(service_id, search_term) +def search_for_notification_by_to_field(service_id, search_term, statuses): + results = notifications_dao.dao_get_notifications_by_to_field(service_id, search_term, statuses) return jsonify( - notifications=notification_with_template_schema.dump(results, many=True).data), 200 + notifications=notification_with_template_schema.dump(results, many=True).data + ), 200 @service_blueprint.route('//notifications/monthly', methods=['GET']) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index b7ca092ba..092b819ad 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -446,7 +446,8 @@ def sample_notification( sent_by=None, client_reference=None, rate_multiplier=1.0, - scheduled_for=None + scheduled_for=None, + normalised_to=None ): if created_at is None: created_at = datetime.utcnow() @@ -484,7 +485,8 @@ def sample_notification( 'sent_by': sent_by, 'updated_at': created_at if status in NOTIFICATION_STATUS_TYPES_COMPLETED else None, 'client_reference': client_reference, - 'rate_multiplier': rate_multiplier + 'rate_multiplier': rate_multiplier, + 'normalised_to': normalised_to } if job_row_number is not None: data['job_row_number'] = job_row_number diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index cbf3eb1b2..28fc0f295 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1622,48 +1622,59 @@ def test_get_monthly_billing_usage_returns_empty_list_if_no_notifications(client def test_search_for_notification_by_to_field(client, notify_db, notify_db_session): - notification1 = create_sample_notification(notify_db, notify_db_session, - to_field="+447700900855") - notification2 = create_sample_notification(notify_db, notify_db_session, to_field="jack@gmail.com") + create_notification = partial(create_sample_notification, notify_db, notify_db_session) + notification1 = create_notification(to_field='+447700900855', normalised_to='447700900855') + notification2 = create_notification(to_field='jack@gmail.com', normalised_to='jack@gmail.com') + + response = client.get( + '/service/{}/notifications?to={}'.format(notification1.service_id, 'jack@gmail.com'), + headers=[create_authorization_header()] + ) + notifications = json.loads(response.get_data(as_text=True))['notifications'] - response = client.get('/service/{}/notifications?to={}'.format(notification1.service_id, "jack@gmail.com"), - headers=[create_authorization_header()]) assert response.status_code == 200 - result = json.loads(response.get_data(as_text=True)) - assert len(result["notifications"]) == 1 - assert result["notifications"][0]["id"] == str(notification2.id) + assert len(notifications) == 1 + assert str(notification2.id) == notifications[0]['id'] def test_search_for_notification_by_to_field_return_empty_list_if_there_is_no_match( - client, notify_db, notify_db_session): - notification1 = create_sample_notification(notify_db, notify_db_session, - to_field="+447700900855") - notification2 = create_sample_notification(notify_db, notify_db_session, to_field="jack@gmail.com") + client, notify_db, notify_db_session +): + create_notification = partial(create_sample_notification, notify_db, notify_db_session) + notification1 = create_notification(to_field='+447700900855') + create_notification(to_field='jack@gmail.com') + + response = client.get( + '/service/{}/notifications?to={}'.format(notification1.service_id, '+447700900800'), + headers=[create_authorization_header()] + ) + notifications = json.loads(response.get_data(as_text=True))['notifications'] - response = client.get('/service/{}/notifications?to={}'.format(notification1.service_id, "+447700900800"), - headers=[create_authorization_header()]) assert response.status_code == 200 - assert len(json.loads(response.get_data(as_text=True))["notifications"]) == 0 + assert len(notifications) == 0 -def test_search_for_notification_by_to_field_return_multiple_matches( - client, notify_db, notify_db_session): - notification1 = create_sample_notification(notify_db, notify_db_session, - to_field="+447700900855") - notification2 = create_sample_notification(notify_db, notify_db_session, - to_field=" +44 77009 00855 ") - notification3 = create_sample_notification(notify_db, notify_db_session, - to_field="+44770 0900 855") - notification4 = create_sample_notification(notify_db, notify_db_session, to_field="jack@gmail.com") +def test_search_for_notification_by_to_field_return_multiple_matches(client, notify_db, notify_db_session): + create_notification = partial(create_sample_notification, notify_db, notify_db_session) + notification1 = create_notification(to_field='+447700900855', normalised_to='447700900855') + notification2 = create_notification(to_field=' +44 77009 00855 ', normalised_to='447700900855') + notification3 = create_notification(to_field='+44770 0900 855', normalised_to='447700900855') + notification4 = create_notification(to_field='jack@gmail.com', normalised_to='jack@gmail.com') + + response = client.get( + '/service/{}/notifications?to={}'.format(notification1.service_id, '+447700900855'), + headers=[create_authorization_header()] + ) + notifications = json.loads(response.get_data(as_text=True))['notifications'] + notification_ids = [notification['id'] for notification in notifications] - response = client.get('/service/{}/notifications?to={}'.format(notification1.service_id, "+447700900855"), - headers=[create_authorization_header()]) assert response.status_code == 200 - result = json.loads(response.get_data(as_text=True)) - assert len(result["notifications"]) == 3 - assert str(notification1.id) in [n["id"] for n in result["notifications"]] - assert str(notification2.id) in [n["id"] for n in result["notifications"]] - assert str(notification3.id) in [n["id"] for n in result["notifications"]] + assert len(notifications) == 3 + + assert str(notification1.id) in notification_ids + assert str(notification2.id) in notification_ids + assert str(notification3.id) in notification_ids + assert str(notification4.id) not in notification_ids def test_update_service_calls_send_notification_as_service_becomes_live(notify_db, notify_db_session, client, mocker): @@ -1751,3 +1762,54 @@ def test_update_service_works_when_sms_sender_is_null(sample_service, client, mo assert resp.status_code == 200 # make sure it wasn't changed to not-null under the hood assert sample_service.sms_sender is None + + +def test_search_for_notification_by_to_field_filters_by_status(client, notify_db, notify_db_session): + create_notification = partial( + create_sample_notification, + notify_db, + notify_db_session, + to_field='+447700900855', + normalised_to='447700900855' + ) + notification1 = create_notification(status='delivered') + create_notification(status='sending') + + response = client.get( + '/service/{}/notifications?to={}&status={}'.format( + notification1.service_id, '+447700900855', 'delivered' + ), + headers=[create_authorization_header()] + ) + notifications = json.loads(response.get_data(as_text=True))['notifications'] + notification_ids = [notification['id'] for notification in notifications] + + assert response.status_code == 200 + assert len(notifications) == 1 + assert str(notification1.id) in notification_ids + + +def test_search_for_notification_by_to_field_filters_by_statuses(client, notify_db, notify_db_session): + create_notification = partial( + create_sample_notification, + notify_db, + notify_db_session, + to_field='+447700900855', + normalised_to='447700900855' + ) + notification1 = create_notification(status='delivered') + notification2 = create_notification(status='sending') + + response = client.get( + '/service/{}/notifications?to={}&status={}&status={}'.format( + notification1.service_id, '+447700900855', 'delivered', 'sending' + ), + headers=[create_authorization_header()] + ) + notifications = json.loads(response.get_data(as_text=True))['notifications'] + notification_ids = [notification['id'] for notification in notifications] + + assert response.status_code == 200 + assert len(notifications) == 2 + assert str(notification1.id) in notification_ids + assert str(notification2.id) in notification_ids From 7aca3d8f434195dcb20410c391c7e390ffe0e68a Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 26 May 2017 12:04:16 +0100 Subject: [PATCH 39/42] Remove flags process in service_dao.create_service --- app/dao/services_dao.py | 11 ----------- tests/app/service/test_rest.py | 4 ++-- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 412723830..572e36e63 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -148,17 +148,6 @@ def dao_create_service(service, user, service_id=None, service_permissions=[SMS_ if permission == LETTER_TYPE: service.can_send_letters = True - def convert_flags(flag, notify_type): - if flag and notify_type not in service_permissions: - service_permission = ServicePermission(service_id=service.id, permission=notify_type) - service.permissions.append(service_permission) - elif flag is False and notify_type in service_permissions: - service_permission = ServicePermission(service_id=service.id, permission=notify_type) - service.permissions.remove(service_permission) - - convert_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE) - convert_flags(service.can_send_letters, LETTER_TYPE) - deprecate_process_service_permissions() db.session.add(service) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 11d50f899..aaacb4ed9 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -501,10 +501,10 @@ def test_update_service_flags_with_service_without_default_service_permissions(c def test_update_service_flags_will_remove_service_permissions(client, notify_db, notify_db_session): auth_header = create_authorization_header() - service = create_service(notify_db, notify_db_session, can_send_international_sms=True) + service = create_service( + notify_db, notify_db_session, permissions=[SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE]) assert service.can_send_international_sms is True - assert set([p.permission for p in service.permissions]) == set([SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE]) data = { 'can_send_international_sms': False From 56e9faab2e62e01ead46eea078b2cd335d8fc449 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 26 May 2017 15:27:49 +0100 Subject: [PATCH 40/42] Refactor schema --- app/schemas.py | 40 ++++++++-------------------------------- 1 file changed, 8 insertions(+), 32 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index ff4f83a95..09301868e 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -184,31 +184,13 @@ class ServiceSchema(BaseSchema): override_flag = False def service_permissions(self, service): - permissions = [] - str_permissions = [] - - perms = dao_fetch_service_permissions(service.id) - for p in perms: - permission = { - "service_id": service.id, - "permission": p.permission - } - permissions.append(permission) - str_permissions.append(p.permission) + permissions = [p.permission for p in service.permissions] def deprecate_convert_flags_to_permissions(): - def convert_flags(flag, notify_type): - if flag and notify_type not in str_permissions: - permission = { - "service_id": service.id, - "permission": notify_type - } + def convert_flags(flag, permission): + if flag and permission not in permissions: permissions.append(permission) - elif flag is False and notify_type in str_permissions: - permission = { - "service_id": service.id, - "permission": notify_type - } + elif flag is False and permission in permissions: permissions.remove(permission) convert_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE) @@ -254,14 +236,13 @@ class ServiceSchema(BaseSchema): if isinstance(in_data, dict) and 'permissions' in in_data: str_permissions = in_data['permissions'] permissions = [] - for p in in_data['permissions']: + for p in str_permissions: permission = ServicePermission(service_id=in_data["id"], permission=p) permissions.append(permission) - in_data['permissions'] = permissions def deprecate_override_flags(): - in_data['can_send_letters'] = LETTER_TYPE in [p.permission for p in permissions] - in_data['can_send_international_sms'] = INTERNATIONAL_SMS_TYPE in [p.permission for p in permissions] + in_data['can_send_letters'] = LETTER_TYPE in str_permissions + in_data['can_send_international_sms'] = INTERNATIONAL_SMS_TYPE in str_permissions def deprecate_convert_flags_to_permissions(): def convert_flags(flag, notify_type): @@ -280,12 +261,7 @@ class ServiceSchema(BaseSchema): deprecate_override_flags() else: deprecate_convert_flags_to_permissions() - - @post_dump - def format_as_string_array(self, in_data): - if isinstance(in_data, dict) and 'permissions' in in_data: - in_data['permissions'] = [p.get("permission") for p in in_data['permissions']] - return in_data + in_data['permissions'] = permissions def set_override_flag(self, flag): self.override_flag = flag From 18b8382d6e0cc6037766d96239043f5b12bee4f5 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 26 May 2017 17:17:15 +0100 Subject: [PATCH 41/42] Refactor schema and improve tests --- app/schemas.py | 12 ------------ tests/app/service/test_rest.py | 12 +++++------- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 09301868e..a89bbcd9f 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -186,18 +186,6 @@ class ServiceSchema(BaseSchema): def service_permissions(self, service): permissions = [p.permission for p in service.permissions] - def deprecate_convert_flags_to_permissions(): - def convert_flags(flag, permission): - if flag and permission not in permissions: - permissions.append(permission) - elif flag is False and permission in permissions: - permissions.remove(permission) - - convert_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE) - convert_flags(service.can_send_letters, LETTER_TYPE) - - deprecate_convert_flags_to_permissions() - return permissions class Meta: diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 7b5fda823..46adbba1b 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -519,7 +519,9 @@ def test_update_service_flags_will_remove_service_permissions(client, notify_db, assert resp.status_code == 200 assert result['data']['can_send_international_sms'] is False - assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE]) + + permissions = ServicePermission.query.filter_by(service_id=service.id).all() + assert set([p.permission for p in permissions]) == set([SMS_TYPE, EMAIL_TYPE]) def test_update_permissions_will_override_permission_flags(client, service_with_no_permissions): @@ -583,14 +585,10 @@ def test_add_service_permission_will_add_permission(client, service_with_no_perm headers=[('Content-Type', 'application/json'), auth_header] ) - resp = client.get( - '/service/{}'.format(service_with_no_permissions.id), - headers=[auth_header] - ) - result = json.loads(resp.get_data(as_text=True)) + permissions = ServicePermission.query.filter_by(service_id=service_with_no_permissions.id).all() assert resp.status_code == 200 - assert result['data']['permissions'] == [permission_to_add] + assert [p.permission for p in permissions] == [permission_to_add] def test_update_permissions_with_an_invalid_permission_will_raise_error(client, sample_service): From 112c6735930801375d45a927a66eaf82d44d9a32 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 26 May 2017 17:23:01 +0100 Subject: [PATCH 42/42] Removed a few lines from schema --- app/schemas.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index a89bbcd9f..b8d3ee7af 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -184,9 +184,7 @@ class ServiceSchema(BaseSchema): override_flag = False def service_permissions(self, service): - permissions = [p.permission for p in service.permissions] - - return permissions + return [p.permission for p in service.permissions] class Meta: model = models.Service