From 0a21f1f3e8ceebc9d56e5b64d807143273c0c4ca Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 11 Sep 2017 11:10:45 +0100 Subject: [PATCH 1/5] Small refactor to how notification_schemas are tested. My local build was not always getting the optional requirement for the jsonschema uri format checker (rfc3987). The requirement has been added to the requirements_for_test file. I changed the tests to validate the response schema from the post_notifications tests, this way we can tell if we are breaking the contract. This showed that the email_from was not returning the entire email address but just the username, that has been corrected here. Removed the response schema validation tests since they are not being testing in the post notification tests. --- app/v2/notifications/notification_schemas.py | 2 +- app/v2/notifications/post_notifications.py | 2 +- requirements_for_test.txt | 2 + .../test_notification_schemas.py | 145 +----------------- .../test_post_letter_notifications.py | 3 + .../notifications/test_post_notifications.py | 8 +- 6 files changed, 16 insertions(+), 146 deletions(-) diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index fefbf5634..a22ad863b 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -177,7 +177,7 @@ email_content = { post_email_response = { "$schema": "http://json-schema.org/draft-04/schema#", - "description": "POST sms notification response schema", + "description": "POST email notification response schema", "type": "object", "title": "response v2/notifications/email", "properties": { diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index db69a5758..c2544ffd4 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -88,7 +88,7 @@ def post_notification(notification_type): create_resp_partial = functools.partial( create_post_email_response_from_notification, subject=template_with_content.subject, - email_from=authenticated_service.email_from + email_from='{}@{}'.format(authenticated_service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']) ) elif notification_type == LETTER_TYPE: create_resp_partial = functools.partial( diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 8bfd763e2..53f76b059 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -8,4 +8,6 @@ coveralls==1.2.0 moto==1.1.2 freezegun==0.3.9 requests-mock==1.3.0 +# optional requirements for jsonschema strict-rfc3339==0.7 +rfc3987==1.3.7 \ No newline at end of file diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 83ad8c5f7..9e3040194 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -5,15 +5,12 @@ from flask import json from freezegun import freeze_time from jsonschema import ValidationError +from app.schema_validation import validate from app.v2.notifications.notification_schemas import ( get_notifications_request, - get_notification_response, post_sms_request as post_sms_request_schema, - post_sms_response as post_sms_response_schema, - post_email_request as post_email_request_schema, - post_email_response as post_email_response_schema + post_email_request as post_email_request_schema ) -from app.schema_validation import validate @pytest.mark.parametrize('invalid_statuses, valid_statuses', [ @@ -163,40 +160,6 @@ def test_post_sms_request_schema_invalid_phone_number_and_missing_template(): assert {"error": "ValidationError", "message": "template_id is a required property"} in errors -def valid_sms_response(): - return { - "id": str(uuid.uuid4()), - "content": {"body": "contents of message", - "from_number": "46045"}, - "uri": "http://notify.api/v2/notifications/id", - "template": { - "id": str(uuid.uuid4()), - "version": 1, - "uri": "http://notify.api/v2/template/id" - } - } - - -def valid_sms_response_with_optionals(): - return { - "id": str(uuid.uuid4()), - "reference": "reference_from_service", - "content": {"body": "contents of message", - "from_number": "46045"}, - "uri": "http://notify.api/v2/notifications/id", - "template": { - "id": str(uuid.uuid4()), - "version": 1, - "uri": "http://notify.api/v2/template/id" - } - } - - -@pytest.mark.parametrize('input', [valid_sms_response()]) -def test_post_sms_response_schema_schema_is_valid(input): - assert validate(input, post_sms_response_schema) == input - - valid_post_email_json = {"email_address": "test@example.gov.uk", "template_id": str(uuid.uuid4()) } @@ -252,110 +215,6 @@ def valid_email_response(): } -def valid_email_response_with_optionals(): - return { - "id": str(uuid.uuid4()), - "reference": "some reference", - "content": {"body": "the body of the message", - "subject": "subject of the message", - "from_email": "service@dig.gov.uk"}, - "uri": "http://notify.api/v2/notifications/id", - "template": { - "id": str(uuid.uuid4()), - "version": 1, - "uri": "http://notify.api/v2/template/id" - }, - "schedule_for": "2017-05-12 13:00:00" - } - - -@pytest.mark.parametrize("input", [valid_email_response(), valid_email_response_with_optionals()]) -def test_post_email_response_schema(input): - assert validate(input, post_email_response_schema) == input - - -@pytest.mark.parametrize('response, schema', [ - (valid_email_response(), post_email_response_schema), - (valid_sms_response(), post_sms_response_schema) -]) -def test_post_sms_response_schema_missing_uri_raises_validation_error(response, schema): - del response['uri'] - with pytest.raises(ValidationError) as e: - validate(response, schema) - error = json.loads(str(e.value)) - assert error['status_code'] == 400 - assert error['errors'] == [{'error': 'ValidationError', - 'message': "uri is a required property"}] - - -@pytest.mark.parametrize('response, schema', [ - (valid_email_response(), post_email_response_schema), - (valid_sms_response(), post_sms_response_schema) -]) -def test_post_sms_response_schema_invalid_uri_raises_validation_error(response, schema): - response['uri'] = 'invalid-uri' - with pytest.raises(ValidationError) as e: - validate(response, schema) - error = json.loads(str(e.value)) - assert error['status_code'] == 400 - assert error['errors'] == [{'error': 'ValidationError', - 'message': "uri invalid-uri is not a valid URI."}] - - -@pytest.mark.parametrize('response, schema', [ - (valid_email_response(), post_email_response_schema), - (valid_sms_response(), post_sms_response_schema) -]) -def test_post_sms_response_schema_missing_template_uri_raises_validation_error(response, schema): - del response['template']['uri'] - with pytest.raises(ValidationError) as e: - validate(response, schema) - error = json.loads(str(e.value)) - assert error['status_code'] == 400 - assert error['errors'] == [{'error': 'ValidationError', - 'message': "template uri is a required property"}] - - -@pytest.mark.parametrize('response, schema', [ - (valid_email_response(), post_email_response_schema), - (valid_sms_response(), post_sms_response_schema) -]) -def test_post_sms_response_schema_invalid_template_uri_raises_validation_error(response, schema): - response['template']['uri'] = 'invalid-uri' - with pytest.raises(ValidationError) as e: - validate(response, schema) - error = json.loads(str(e.value)) - assert error['status_code'] == 400 - assert error['errors'] == [{'error': 'ValidationError', - 'message': "template invalid-uri is not a valid URI."}] - - -def test_get_notifications_response_with_email_and_phone_number(): - response = {"id": str(uuid.uuid4()), - "reference": "something", - "email_address": None, - "phone_number": "+447115411111", - "line_1": None, - "line_2": None, - "line_3": None, - "line_4": None, - "line_5": None, - "line_6": None, - "postcode": None, - "type": "email", - "status": "delivered", - "template": {"id": str(uuid.uuid4()), "version": 1, "uri": "http://template/id"}, - "body": "some body", - "subject": "some subject", - "created_at": "2016-01-01", - "sent_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]) @freeze_time("2017-05-12 13:00:00") diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index aa4433a59..2f883d1be 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -12,7 +12,9 @@ from app.models import KEY_TYPE_TEST from app.models import LETTER_TYPE from app.models import Notification from app.models import SMS_TYPE +from app.schema_validation import validate from app.v2.errors import RateLimitError +from app.v2.notifications.notification_schemas import post_letter_response from app.variables import LETTER_TEST_API_FILENAME from app.variables import LETTER_API_FILENAME @@ -53,6 +55,7 @@ def test_post_letter_notification_returns_201(client, sample_letter_template, mo resp_json = letter_request(client, data, service_id=sample_letter_template.service_id) + assert validate(resp_json, post_letter_response) == resp_json job = Job.query.one() assert job.original_file_name == LETTER_API_FILENAME notification = Notification.query.one() diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index bd5b4f711..4e72389c3 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -10,7 +10,9 @@ from app.models import ( from flask import json, current_app from app.models import Notification +from app.schema_validation import validate from app.v2.errors import RateLimitError +from app.v2.notifications.notification_schemas import post_sms_response, post_email_response from tests import create_authorization_header from tests.app.conftest import ( sample_template as create_sample_template, sample_service, @@ -38,6 +40,7 @@ def test_post_sms_notification_returns_201(client, sample_template_with_placehol headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 201 resp_json = json.loads(response.get_data(as_text=True)) + assert validate(resp_json, post_sms_response) == resp_json notifications = Notification.query.all() assert len(notifications) == 1 notification_id = notifications[0].id @@ -71,6 +74,7 @@ def test_post_sms_notification_uses_inbound_number_as_sender(client, sample_temp headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 201 resp_json = json.loads(response.get_data(as_text=True)) + assert validate(resp_json, post_sms_response) == resp_json notifications = Notification.query.all() assert len(notifications) == 1 notification_id = notifications[0].id @@ -170,6 +174,7 @@ def test_post_email_notification_returns_201(client, sample_email_template_with_ headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 201 resp_json = json.loads(response.get_data(as_text=True)) + assert validate(resp_json, post_email_response) == resp_json notification = Notification.query.first() assert resp_json['id'] == str(notification.id) assert resp_json['reference'] == reference @@ -178,7 +183,8 @@ def test_post_email_notification_returns_201(client, sample_email_template_with_ .replace('((name))', 'Bob').replace('GOV.UK', u'GOV.\u200bUK') assert resp_json['content']['subject'] == sample_email_template_with_placeholders.subject \ .replace('((name))', 'Bob') - assert resp_json['content']['from_email'] == sample_email_template_with_placeholders.service.email_from + assert resp_json['content']['from_email'] == "{}@{}".format( + sample_email_template_with_placeholders.service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']) assert 'v2/notifications/{}'.format(notification.id) in resp_json['uri'] assert resp_json['template']['id'] == str(sample_email_template_with_placeholders.id) assert resp_json['template']['version'] == sample_email_template_with_placeholders.version From 43bda1ae2cf618948c70c6a5cd6a4bb77f14d66c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 11 Sep 2017 15:47:10 +0100 Subject: [PATCH 2/5] upgrade flask-bcrypt to 0.7.1 this uses bcrypt directly rather than using the wrapper python-bcrypt. WARNING! You'll need to update your local install by running: ``` pip uninstall python-bcrypt pip install flask-bcrypt==0.7.1 ``` --- app/encryption.py | 2 +- requirements.txt | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/app/encryption.py b/app/encryption.py index 34577c0d6..34ba6df8e 100644 --- a/app/encryption.py +++ b/app/encryption.py @@ -16,7 +16,7 @@ class Encryption: def hashpw(password): - return generate_password_hash(password.encode('UTF-8'), 10) + return generate_password_hash(password.encode('UTF-8'), 10).decode('utf-8') def check_hash(password, hashed_password): diff --git a/requirements.txt b/requirements.txt index a1ef0f9eb..7df31e888 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,10 +1,7 @@ boto3==1.4.7 celery==3.1.25 # pyup: <4 docopt==0.6.2 -# ignore flask-bcrypt - when upgrading, it installs an invalid version of bcrypt that isn't removed when a different -# branch runs, so can cause other PR builds to fail on jenkins. -# TODO: Upgrade flask-bcrypt in a safe way -Flask-Bcrypt==0.6.2 # pyup: ignore +Flask-Bcrypt==0.7.1 Flask-Marshmallow==0.8.0 Flask-Migrate==2.1.1 Flask-Script==2.0.5 From e912527a61783980ea30a389a76eac457a4f8b6c Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Mon, 11 Sep 2017 20:48:22 +0100 Subject: [PATCH 3/5] Update moto from 1.1.2 to 1.1.4 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 53f76b059..ef46904fc 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -5,7 +5,7 @@ pytest-mock==1.6.2 pytest-cov==2.5.1 pytest-xdist==1.20.0 coveralls==1.2.0 -moto==1.1.2 +moto==1.1.4 freezegun==0.3.9 requests-mock==1.3.0 # optional requirements for jsonschema From 269e88f7de86a391c632df6735d88c64c5ce7fd8 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 11 Sep 2017 16:28:44 +0100 Subject: [PATCH 4/5] Update reply to email test - Updated reply to email test so that the request to create or update email is not mocked. - Excluded reply_to_email_addresses from the Service schema --- app/schemas.py | 1 + tests/app/service/test_rest.py | 8 +++----- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 5f81821d4..3ba27d4d3 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -202,6 +202,7 @@ class ServiceSchema(BaseSchema): 'template_statistics', 'service_provider_stats', 'service_notification_stats', + 'reply_to_email_addresses', ) strict = True diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 0d8e3fea2..186a5d974 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -12,7 +12,7 @@ from app.dao.services_dao import dao_remove_user_from_service from app.dao.templates_dao import dao_redact_template from app.dao.users_dao import save_model_user from app.models import ( - User, Organisation, Service, ServicePermission, Notification, + User, Organisation, Service, ServicePermission, Notification, ServiceEmailReplyTo, DVLA_ORG_LAND_REGISTRY, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE, @@ -2125,9 +2125,7 @@ def test_is_service_name_unique_returns_400_when_name_does_not_exist(client): assert json_resp["message"][1]["email_from"] == ["Can't be empty"] -def test_update_service_reply_to_email_address_upserts_email_reply_to(mocker, admin_request, sample_service): - update_mock = mocker.patch('app.service.rest.create_or_update_email_reply_to') - +def test_update_service_reply_to_email_address_upserts_email_reply_to(admin_request, sample_service): admin_request.post( 'service.update_service', service_id=sample_service.id, @@ -2137,4 +2135,4 @@ def test_update_service_reply_to_email_address_upserts_email_reply_to(mocker, ad _expected_status=200 ) - assert update_mock.called + assert len(ServiceEmailReplyTo.query.all()) == 1 From d356957974c9f04fd94f9f99563bcbbe62e53e78 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 12 Sep 2017 09:50:52 +0100 Subject: [PATCH 5/5] Change the ServiceEmailReplyTo model to uselist for the backref to services (allow 0 to many reply to emails) Add another assert in the test --- app/models.py | 2 +- tests/app/service/test_rest.py | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/models.py b/app/models.py index 588c95d48..4f8e719c9 100644 --- a/app/models.py +++ b/app/models.py @@ -1337,7 +1337,7 @@ class ServiceEmailReplyTo(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), unique=False, index=True, nullable=False) - service = db.relationship(Service, backref=db.backref("reply_to_email_addresses", uselist=False)) + service = db.relationship(Service, backref=db.backref("reply_to_email_addresses")) email_address = db.Column(db.Text, nullable=False, index=False, unique=False) is_default = db.Column(db.Boolean, nullable=False, default=True) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 186a5d974..15a105363 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2126,7 +2126,7 @@ def test_is_service_name_unique_returns_400_when_name_does_not_exist(client): def test_update_service_reply_to_email_address_upserts_email_reply_to(admin_request, sample_service): - admin_request.post( + response = admin_request.post( 'service.update_service', service_id=sample_service.id, _data={ @@ -2135,4 +2135,8 @@ def test_update_service_reply_to_email_address_upserts_email_reply_to(admin_requ _expected_status=200 ) - assert len(ServiceEmailReplyTo.query.all()) == 1 + service_reply_to_emails = ServiceEmailReplyTo.query.all() + assert len(service_reply_to_emails) == 1 + assert service_reply_to_emails[0].email_address == 'new@mail.com' + assert service_reply_to_emails[0].is_default + assert response['data']['reply_to_email_address'] == 'new@mail.com'