From 9f6c037530235e5a47b945ed6ade2ec3de393544 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 24 May 2017 16:27:15 +0100 Subject: [PATCH] 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.'}]