From 90de46bbb607bf9e9f07624495f86e5eb4723cf0 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Fri, 3 Nov 2017 21:24:48 +0000 Subject: [PATCH 01/35] Update sqlalchemy from 1.1.14 to 1.1.15 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 8b9d8624f..0b9a65368 100644 --- a/requirements.txt +++ b/requirements.txt @@ -17,7 +17,7 @@ monotonic==1.4 psycopg2==2.7.3.2 PyJWT==1.5.3 SQLAlchemy-Utils==0.32.19 -SQLAlchemy==1.1.14 +SQLAlchemy==1.1.15 statsd==3.2.1 notifications-python-client==4.5.0 From 85b8e24e17d8f71029d191547648dc1df69f4c67 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 3 Nov 2017 16:35:22 +0000 Subject: [PATCH 02/35] Add V2 inbound_sms API - had to update the serialization in the model so that the date time is appended with the UTC timezone - test has been added to ensure that the schema will validate the response correctly --- app/__init__.py | 4 + app/models.py | 4 +- app/v2/inbound_sms/__init__.py | 6 + app/v2/inbound_sms/get_inbound_sms.py | 36 +++++ app/v2/inbound_sms/inbound_sms_schemas.py | 51 +++++++ tests/app/conftest.py | 6 + tests/app/db.py | 6 +- tests/app/v2/inbound_sms/__init__.py | 0 .../v2/inbound_sms/test_get_inbound_sms.py | 131 ++++++++++++++++++ .../inbound_sms/test_inbound_sms_schemas.py | 75 ++++++++++ 10 files changed, 315 insertions(+), 4 deletions(-) create mode 100644 app/v2/inbound_sms/__init__.py create mode 100644 app/v2/inbound_sms/get_inbound_sms.py create mode 100644 app/v2/inbound_sms/inbound_sms_schemas.py create mode 100644 tests/app/v2/inbound_sms/__init__.py create mode 100644 tests/app/v2/inbound_sms/test_get_inbound_sms.py create mode 100644 tests/app/v2/inbound_sms/test_inbound_sms_schemas.py diff --git a/app/__init__.py b/app/__init__.py index 31a601f3c..b93abfe6e 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -174,6 +174,7 @@ def register_blueprint(application): def register_v2_blueprints(application): + from app.v2.inbound_sms.get_inbound_sms import v2_inbound_sms_blueprint as get_inbound_sms from app.v2.notifications.post_notifications import v2_notification_blueprint as post_notifications from app.v2.notifications.get_notifications import v2_notification_blueprint as get_notifications from app.v2.template.get_template import v2_template_blueprint as get_template @@ -196,6 +197,9 @@ def register_v2_blueprints(application): post_template.before_request(requires_auth) application.register_blueprint(post_template) + get_inbound_sms.before_request(requires_auth) + application.register_blueprint(get_inbound_sms) + def init_app(app): @app.before_request diff --git a/app/models.py b/app/models.py index 74b0af79a..ea7b93a4e 100644 --- a/app/models.py +++ b/app/models.py @@ -1405,12 +1405,12 @@ class InboundSms(db.Model): def serialize(self): return { 'id': str(self.id), - 'created_at': self.created_at.isoformat(), + 'created_at': self.created_at.strftime(DATETIME_FORMAT), 'service_id': str(self.service_id), 'notify_number': self.notify_number, 'user_number': self.user_number, 'content': self.content, - 'provider_date': self.provider_date and self.provider_date.isoformat(), + 'provider_date': self.provider_date and self.provider_date.strftime(DATETIME_FORMAT), 'provider_reference': self.provider_reference } diff --git a/app/v2/inbound_sms/__init__.py b/app/v2/inbound_sms/__init__.py new file mode 100644 index 000000000..d48a37f94 --- /dev/null +++ b/app/v2/inbound_sms/__init__.py @@ -0,0 +1,6 @@ +from flask import Blueprint +from app.v2.errors import register_errors + +v2_inbound_sms_blueprint = Blueprint("v2_inbound_sms", __name__, url_prefix='/v2/inbound_sms') + +register_errors(v2_inbound_sms_blueprint) diff --git a/app/v2/inbound_sms/get_inbound_sms.py b/app/v2/inbound_sms/get_inbound_sms.py new file mode 100644 index 000000000..10ae26885 --- /dev/null +++ b/app/v2/inbound_sms/get_inbound_sms.py @@ -0,0 +1,36 @@ +import uuid + +from flask import jsonify, request, url_for, current_app +from sqlalchemy.orm.exc import NoResultFound +from werkzeug.exceptions import abort + +from notifications_utils.recipients import validate_and_format_phone_number +from notifications_utils.recipients import InvalidPhoneError + +from app import authenticated_service +from app.dao import inbound_sms_dao +from app.v2.errors import BadRequestError +from app.v2.inbound_sms import v2_inbound_sms_blueprint + + +@v2_inbound_sms_blueprint.route("/", methods=['GET']) +def get_inbound_sms_by_number(user_number): + try: + validate_and_format_phone_number(user_number) + except InvalidPhoneError as e: + raise BadRequestError(message=str(e)) + + inbound_sms = inbound_sms_dao.dao_get_inbound_sms_for_service( + authenticated_service.id, user_number=user_number + ) + + return jsonify(inbound_sms_list=[i.serialize() for i in inbound_sms]), 200 + + +@v2_inbound_sms_blueprint.route("", methods=['GET']) +def get_all_inbound_sms(): + all_inbound_sms = inbound_sms_dao.dao_get_inbound_sms_for_service( + authenticated_service.id + ) + + return jsonify(inbound_sms_list=[i.serialize() for i in all_inbound_sms]), 200 diff --git a/app/v2/inbound_sms/inbound_sms_schemas.py b/app/v2/inbound_sms/inbound_sms_schemas.py new file mode 100644 index 000000000..8dedda195 --- /dev/null +++ b/app/v2/inbound_sms/inbound_sms_schemas.py @@ -0,0 +1,51 @@ +from app.schema_validation.definitions import uuid + + +get_inbound_sms_single_response = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "GET inbound sms schema response", + "type": "object", + "title": "GET response v2/inbound_sms", + "properties": { + "provider_date": { + "format": "date-time", + "type": "string", + "description": "Date+time sent by provider" + }, + "provider_reference": {"type": ["string", "null"]}, + "user_number": {"type": "string"}, + "created_at": { + "format": "date-time", + "type": "string", + "description": "Date+time created at" + }, + "service_id": uuid, + "id": uuid, + "notify_number": {"type": "string"}, + "content": {"type": "string"}, + }, + "required": [ + "id", "provider_date", "provider_reference", + "user_number", "created_at", "service_id", + "notify_number", "content" + ], +} + +get_inbound_sms_response = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "GET list of inbound sms response schema", + "type": "object", + "properties": { + "inbound_sms_list": { + "type": "array", + "items": { + "type": "object", + "$ref": "#/definitions/inbound_sms" + } + }, + }, + "required": ["inbound_sms_list"], + "definitions": { + "inbound_sms": get_inbound_sms_single_response + } +} diff --git a/tests/app/conftest.py b/tests/app/conftest.py index e03b2b7dd..081cdf455 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -47,6 +47,7 @@ from tests.app.db import ( create_api_key, create_inbound_number, create_letter_contact, + create_inbound_sms, ) @@ -1014,6 +1015,11 @@ def sample_inbound_numbers(notify_db, notify_db_session, sample_service): return inbound_numbers +@pytest.fixture +def sample_inbound_sms(notify_db, notify_db_session, sample_service): + return create_inbound_sms(sample_service) + + @pytest.fixture def restore_provider_details(notify_db, notify_db_session): """ diff --git a/tests/app/db.py b/tests/app/db.py index c7c950544..cb7729244 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,6 +1,8 @@ from datetime import datetime +import pytz import uuid +from app import DATETIME_FORMAT from app import db from app.dao.jobs_dao import dao_create_job from app.dao.service_inbound_api_dao import save_service_inbound_api @@ -247,10 +249,10 @@ def create_inbound_sms( ): inbound = InboundSms( service=service, - created_at=created_at or datetime.utcnow(), + created_at=created_at or datetime.utcnow().strftime(DATETIME_FORMAT), notify_number=notify_number or service.sms_sender, user_number=user_number, - provider_date=provider_date or datetime.utcnow(), + provider_date=provider_date or datetime.utcnow().strftime(DATETIME_FORMAT), provider_reference=provider_reference or 'foo', content=content, provider=provider diff --git a/tests/app/v2/inbound_sms/__init__.py b/tests/app/v2/inbound_sms/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/app/v2/inbound_sms/test_get_inbound_sms.py b/tests/app/v2/inbound_sms/test_get_inbound_sms.py new file mode 100644 index 000000000..028692414 --- /dev/null +++ b/tests/app/v2/inbound_sms/test_get_inbound_sms.py @@ -0,0 +1,131 @@ +import datetime +import pytest +from flask import json, url_for + +from app import DATETIME_FORMAT +from tests import create_authorization_header +from tests.app.db import create_inbound_sms + + +def test_get_all_inbound_sms_returns_200( + client, sample_service +): + all_inbound_sms = [ + create_inbound_sms(service=sample_service, user_number='447700900111', content='Hi'), + create_inbound_sms(service=sample_service, user_number='447700900112'), + create_inbound_sms(service=sample_service, user_number='447700900111', content='Bye'), + create_inbound_sms(service=sample_service, user_number='447700900113') + ] + + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.get( + path='/v2/inbound_sms', + 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))['inbound_sms_list'] + + reversed_all_inbound_sms = sorted(all_inbound_sms, key=lambda sms: sms.created_at, reverse=True) + + expected_response = [i.serialize() for i in reversed_all_inbound_sms] + + assert json_response == expected_response + + +def test_get_all_inbound_sms_for_no_inbound_sms_returns_200( + client, sample_service +): + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.get( + path='/v2/inbound_sms', + 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))['inbound_sms_list'] + + expected_response = [] + + assert json_response == expected_response + + +def test_get_inbound_sms_by_number_returns_200( + client, sample_service +): + sample_inbound_sms = create_inbound_sms(service=sample_service) + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.get( + path='/v2/inbound_sms/{}'.format(sample_inbound_sms.user_number), + 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))['inbound_sms_list'] + + expected_response = [sample_inbound_sms.serialize()] + + assert json_response == expected_response + + +def test_get_inbound_sms_for_no_inbound_sms_returns_200( + client, sample_service +): + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.get( + path='/v2/inbound_sms', + 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))['inbound_sms_list'] + + expected_response = [] + + assert json_response == expected_response + + +def test_get_inbound_sms_by_nonexistent_number(client, sample_service): + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.get( + path='/v2/inbound_sms/447700900000', + 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))['inbound_sms_list'] + expected_response = [] + + assert json_response == expected_response + + +@pytest.mark.parametrize('invalid_number,expected_message', [ + ('0077700', 'Not enough digits'), + ('123456789012', 'Not a UK mobile number'), + ('invalid_number', 'Must not contain letters or symbols') +]) +def test_get_inbound_sms_by_invalid_number( + client, sample_service, invalid_number, expected_message): + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.get( + path='/v2/inbound_sms/{}'.format(invalid_number), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 400 + assert response.headers['Content-type'] == 'application/json' + + json_response = json.loads(response.get_data(as_text=True)) + assert json_response == { + "errors": [ + { + "error": "BadRequestError", + "message": expected_message + } + ], + "status_code": 400 + } diff --git a/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py new file mode 100644 index 000000000..04e61a321 --- /dev/null +++ b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py @@ -0,0 +1,75 @@ +import uuid + +import pytest +from flask import json +from jsonschema.exceptions import ValidationError + +from app.dao.api_key_dao import save_model_api_key +from app.models import ApiKey, KEY_TYPE_NORMAL, EMAIL_TYPE, SMS_TYPE, TEMPLATE_TYPES +from app.v2.inbound_sms.inbound_sms_schemas import get_inbound_sms_response, get_inbound_sms_single_response +from app.schema_validation import validate + +from tests import create_authorization_header + + +valid_inbound_sms = { + "provider_date": "2017-11-02T15:07:57.199541Z", + "provider_reference": "foo", + "user_number": "447700900111", + "created_at": "2017-11-02T15:07:57.197546Z", + "service_id": "a5149c32-f03b-4711-af49-ad6993797d45", + "id": "342786aa-23ce-4695-9aad-7f79e68ee29a", + "notify_number": "testing", + "content": "Hello" +} + +valid_inbound_sms_list = { + "inbound_sms_list": [valid_inbound_sms] +} + +invalid_inbound_sms = { + "provider_date": "2017-11-02T15:07:57.199541", + "provider_reference": "foo", + "user_number": "447700900111", + "created_at": "2017-11-02T15:07:57.197546", + "service_id": "a5149c32-f03b-4711-af49-ad6993797d45", + "id": "342786aa-23ce-4695-9aad-7f79e68ee29a", + "notify_number": "testing" +} + +invalid_inbound_sms_list = { + "inbound_sms_list": [invalid_inbound_sms] +} + + +def _get_inbound_sms(client, inbound_sms, url): + auth_header = create_authorization_header(service_id=inbound_sms.service_id) + response = client.get(url, headers=[auth_header]) + return json.loads(response.get_data(as_text=True)) + + +def test_get_inbound_sms_contract(client, sample_inbound_sms): + response_json = _get_inbound_sms( + client, + sample_inbound_sms, + '/v2/inbound_sms/{}'.format(sample_inbound_sms.user_number) + ) + res = validate(response_json, get_inbound_sms_response) + + +def test_valid_inbound_sms_json(): + validate(valid_inbound_sms, get_inbound_sms_single_response) + + +def test_valid_inbound_sms_list_json(): + validate(valid_inbound_sms_list, get_inbound_sms_response) + + +def test_invalid_inbound_sms_json(): + with pytest.raises(expected_exception=ValidationError): + validate(invalid_inbound_sms, get_inbound_sms_single_response) + + +def test_invalid_inbound_sms_list_json(): + with pytest.raises(expected_exception=ValidationError): + validate(invalid_inbound_sms_list, get_inbound_sms_response) From 1c673a4afe2a1aac8fb9679455ec84c0ee582619 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 3 Nov 2017 17:26:53 +0000 Subject: [PATCH 03/35] Refactor tests --- tests/app/db.py | 6 ++--- .../inbound_sms/test_inbound_sms_schemas.py | 24 ++++++------------- 2 files changed, 9 insertions(+), 21 deletions(-) diff --git a/tests/app/db.py b/tests/app/db.py index cb7729244..c7c950544 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,8 +1,6 @@ from datetime import datetime -import pytz import uuid -from app import DATETIME_FORMAT from app import db from app.dao.jobs_dao import dao_create_job from app.dao.service_inbound_api_dao import save_service_inbound_api @@ -249,10 +247,10 @@ def create_inbound_sms( ): inbound = InboundSms( service=service, - created_at=created_at or datetime.utcnow().strftime(DATETIME_FORMAT), + created_at=created_at or datetime.utcnow(), notify_number=notify_number or service.sms_sender, user_number=user_number, - provider_date=provider_date or datetime.utcnow().strftime(DATETIME_FORMAT), + provider_date=provider_date or datetime.utcnow(), provider_reference=provider_reference or 'foo', content=content, provider=provider diff --git a/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py index 04e61a321..9a7f654ba 100644 --- a/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py +++ b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py @@ -1,11 +1,7 @@ -import uuid - import pytest from flask import json from jsonschema.exceptions import ValidationError -from app.dao.api_key_dao import save_model_api_key -from app.models import ApiKey, KEY_TYPE_NORMAL, EMAIL_TYPE, SMS_TYPE, TEMPLATE_TYPES from app.v2.inbound_sms.inbound_sms_schemas import get_inbound_sms_response, get_inbound_sms_single_response from app.schema_validation import validate @@ -42,23 +38,17 @@ invalid_inbound_sms_list = { } -def _get_inbound_sms(client, inbound_sms, url): - auth_header = create_authorization_header(service_id=inbound_sms.service_id) - response = client.get(url, headers=[auth_header]) - return json.loads(response.get_data(as_text=True)) - - def test_get_inbound_sms_contract(client, sample_inbound_sms): - response_json = _get_inbound_sms( - client, - sample_inbound_sms, - '/v2/inbound_sms/{}'.format(sample_inbound_sms.user_number) - ) - res = validate(response_json, get_inbound_sms_response) + auth_header = create_authorization_header(service_id=sample_inbound_sms.service_id) + response = client.get('/v2/inbound_sms/{}'.format(sample_inbound_sms.user_number), headers=[auth_header]) + response_json = json.loads(response.get_data(as_text=True)) + + assert validate(response_json, get_inbound_sms_response)['inbound_sms_list'][0] \ + == sample_inbound_sms.serialize() def test_valid_inbound_sms_json(): - validate(valid_inbound_sms, get_inbound_sms_single_response) + assert validate(valid_inbound_sms, get_inbound_sms_single_response) == valid_inbound_sms def test_valid_inbound_sms_list_json(): From 02ee072251a3b1ac24d468c84cb730655e4f77b5 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 6 Nov 2017 18:19:02 +0000 Subject: [PATCH 04/35] Refactor inbound sms dao Added function for paginated results --- app/dao/inbound_sms_dao.py | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 18060cced..5bbc9a3a9 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -2,7 +2,8 @@ from datetime import ( timedelta, datetime ) - +from flask import current_app +from sqlalchemy import desc from app import db from app.dao.dao_utils import transactional @@ -31,6 +32,34 @@ def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None): return q.all() +def dao_get_paginated_inbound_sms_for_service( + service_id, + user_number=None, + older_than=None, + page=1, + page_size=None +): + if page_size is None: + page_size = current_app.config['PAGE_SIZE'] + + filters = [InboundSms.service_id == service_id] + + if older_than is not None: + older_than_created_at = db.session.query( + InboundSms.created_at).filter(InboundSms.id == older_than).as_scalar() + filters.append(InboundSms.created_at < older_than_created_at) + + if user_number: + filters.append(InboundSms.user_number == user_number) + + query = InboundSms.query.filter(*filters) + + return query.order_by(desc(InboundSms.created_at)).paginate( + page=page, + per_page=page_size + ).items + + def dao_count_inbound_sms_for_service(service_id): return InboundSms.query.filter( InboundSms.service_id == service_id From 0350c99a3e99e35b0de10cf29616b139e3b57b8f Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 6 Nov 2017 18:22:18 +0000 Subject: [PATCH 05/35] Updated inbound sms API endpoints - from `/v2/inbound_sms` to `/v2/received-text-messages` - use paginated results --- app/v2/inbound_sms/__init__.py | 2 +- app/v2/inbound_sms/get_inbound_sms.py | 61 ++++++++++++++++--- app/v2/inbound_sms/inbound_sms_schemas.py | 15 ++++- .../v2/inbound_sms/test_get_inbound_sms.py | 56 ++++++++++++++--- .../inbound_sms/test_inbound_sms_schemas.py | 8 ++- 5 files changed, 122 insertions(+), 20 deletions(-) diff --git a/app/v2/inbound_sms/__init__.py b/app/v2/inbound_sms/__init__.py index d48a37f94..5c713b2ef 100644 --- a/app/v2/inbound_sms/__init__.py +++ b/app/v2/inbound_sms/__init__.py @@ -1,6 +1,6 @@ from flask import Blueprint from app.v2.errors import register_errors -v2_inbound_sms_blueprint = Blueprint("v2_inbound_sms", __name__, url_prefix='/v2/inbound_sms') +v2_inbound_sms_blueprint = Blueprint("v2_inbound_sms", __name__, url_prefix='/v2/received-text-messages') register_errors(v2_inbound_sms_blueprint) diff --git a/app/v2/inbound_sms/get_inbound_sms.py b/app/v2/inbound_sms/get_inbound_sms.py index 10ae26885..1ebae90c5 100644 --- a/app/v2/inbound_sms/get_inbound_sms.py +++ b/app/v2/inbound_sms/get_inbound_sms.py @@ -15,22 +15,69 @@ from app.v2.inbound_sms import v2_inbound_sms_blueprint @v2_inbound_sms_blueprint.route("/", methods=['GET']) def get_inbound_sms_by_number(user_number): + _data = request.args.to_dict(flat=False) + + # flat=False makes everything a list, but we only ever allow one value for "older_than" + if 'older_than' in _data: + _data['older_than'] = _data['older_than'][0] + try: - validate_and_format_phone_number(user_number) + user_number = validate_and_format_phone_number(user_number) except InvalidPhoneError as e: raise BadRequestError(message=str(e)) - inbound_sms = inbound_sms_dao.dao_get_inbound_sms_for_service( - authenticated_service.id, user_number=user_number + paginated_inbound_sms = inbound_sms_dao.dao_get_paginated_inbound_sms_for_service( + authenticated_service.id, + user_number=user_number, + older_than=_data.get('older_than'), + page_size=current_app.config.get('API_PAGE_SIZE') ) - return jsonify(inbound_sms_list=[i.serialize() for i in inbound_sms]), 200 + return jsonify( + inbound_sms_list=[i.serialize() for i in paginated_inbound_sms], + links=_build_links( + paginated_inbound_sms, + endpoint='get_inbound_sms_by_number', + user_number=user_number + ) + ), 200 @v2_inbound_sms_blueprint.route("", methods=['GET']) def get_all_inbound_sms(): - all_inbound_sms = inbound_sms_dao.dao_get_inbound_sms_for_service( - authenticated_service.id + _data = request.args.to_dict(flat=False) + + # flat=False makes everything a list, but we only ever allow one value for "older_than" + if 'older_than' in _data: + _data['older_than'] = _data['older_than'][0] + + paginated_inbound_sms = inbound_sms_dao.dao_get_paginated_inbound_sms_for_service( + authenticated_service.id, + older_than=_data.get('older_than'), + page_size=current_app.config.get('API_PAGE_SIZE') ) - return jsonify(inbound_sms_list=[i.serialize() for i in all_inbound_sms]), 200 + return jsonify( + inbound_sms_list=[i.serialize() for i in paginated_inbound_sms], + links=_build_links(paginated_inbound_sms, endpoint='get_all_inbound_sms') + ), 200 + + +def _build_links(inbound_sms_list, endpoint, user_number=None): + _links = { + 'current': url_for( + "v2_inbound_sms.{}".format(endpoint), + user_number=user_number, + _external=True, + ), + } + + if inbound_sms_list: + _links['next'] = url_for( + "v2_inbound_sms.{}".format(endpoint), + user_number=user_number, + older_than=inbound_sms_list[-1].id, + _external=True, + ) + + return _links diff --git a/app/v2/inbound_sms/inbound_sms_schemas.py b/app/v2/inbound_sms/inbound_sms_schemas.py index 8dedda195..0990c1e2e 100644 --- a/app/v2/inbound_sms/inbound_sms_schemas.py +++ b/app/v2/inbound_sms/inbound_sms_schemas.py @@ -43,8 +43,21 @@ get_inbound_sms_response = { "$ref": "#/definitions/inbound_sms" } }, + "links": { + "type": "object", + "properties": { + "current": { + "type": "string" + }, + "next": { + "type": "string" + } + }, + "additionalProperties": False, + "required": ["current"] + } }, - "required": ["inbound_sms_list"], + "required": ["inbound_sms_list", "links"], "definitions": { "inbound_sms": get_inbound_sms_single_response } diff --git a/tests/app/v2/inbound_sms/test_get_inbound_sms.py b/tests/app/v2/inbound_sms/test_get_inbound_sms.py index 028692414..34eb0420a 100644 --- a/tests/app/v2/inbound_sms/test_get_inbound_sms.py +++ b/tests/app/v2/inbound_sms/test_get_inbound_sms.py @@ -14,12 +14,12 @@ def test_get_all_inbound_sms_returns_200( create_inbound_sms(service=sample_service, user_number='447700900111', content='Hi'), create_inbound_sms(service=sample_service, user_number='447700900112'), create_inbound_sms(service=sample_service, user_number='447700900111', content='Bye'), - create_inbound_sms(service=sample_service, user_number='447700900113') + create_inbound_sms(service=sample_service, user_number='07700900113') ] auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path='/v2/inbound_sms', + path='/v2/received-text-messages', headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 @@ -34,12 +34,47 @@ def test_get_all_inbound_sms_returns_200( assert json_response == expected_response +@pytest.mark.parametrize('inbound_sms_path,user_number', [ + ('v2_inbound_sms.get_all_inbound_sms', None), + ('v2_inbound_sms.get_inbound_sms_by_number', '447700900111') +]) +def test_get_all_inbound_sms_generate_page_links( + client, sample_service, mocker, inbound_sms_path, user_number +): + mocker.patch.dict("app.v2.inbound_sms.get_inbound_sms.current_app.config", {"API_PAGE_SIZE": 1}) + all_inbound_sms = [ + create_inbound_sms(service=sample_service, user_number='447700900111', content='Hi'), + create_inbound_sms(service=sample_service, user_number='447700900111'), + ] + + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.get( + path=url_for(inbound_sms_path, user_number=user_number), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 200 + + json_response = json.loads(response.get_data(as_text=True)) + expected_inbound_sms_list = [all_inbound_sms[-1].serialize()] + + assert json_response['inbound_sms_list'] == expected_inbound_sms_list + assert url_for( + inbound_sms_path, + user_number=user_number, + _external=True) == json_response['links']['current'] + assert url_for( + inbound_sms_path, + user_number=user_number, + older_than=all_inbound_sms[-1].id, + _external=True) == json_response['links']['next'] + + def test_get_all_inbound_sms_for_no_inbound_sms_returns_200( client, sample_service ): auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path='/v2/inbound_sms', + path='/v2/received-text-messages', headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 @@ -55,10 +90,13 @@ def test_get_all_inbound_sms_for_no_inbound_sms_returns_200( def test_get_inbound_sms_by_number_returns_200( client, sample_service ): - sample_inbound_sms = create_inbound_sms(service=sample_service) + sample_inbound_sms1 = create_inbound_sms(service=sample_service, user_number='447700900111') + create_inbound_sms(service=sample_service, user_number='447700900112') + sample_inbound_sms2 = create_inbound_sms(service=sample_service, user_number='447700900111') + auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path='/v2/inbound_sms/{}'.format(sample_inbound_sms.user_number), + path='/v2/received-text-messages/{}'.format('07700900111'), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 @@ -66,7 +104,7 @@ def test_get_inbound_sms_by_number_returns_200( json_response = json.loads(response.get_data(as_text=True))['inbound_sms_list'] - expected_response = [sample_inbound_sms.serialize()] + expected_response = [sample_inbound_sms2.serialize(), sample_inbound_sms1.serialize()] assert json_response == expected_response @@ -76,7 +114,7 @@ def test_get_inbound_sms_for_no_inbound_sms_returns_200( ): auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path='/v2/inbound_sms', + path='/v2/received-text-messages', headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 @@ -92,7 +130,7 @@ def test_get_inbound_sms_for_no_inbound_sms_returns_200( def test_get_inbound_sms_by_nonexistent_number(client, sample_service): auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path='/v2/inbound_sms/447700900000', + path='/v2/received-text-messages/447700900000', headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 @@ -113,7 +151,7 @@ def test_get_inbound_sms_by_invalid_number( client, sample_service, invalid_number, expected_message): auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path='/v2/inbound_sms/{}'.format(invalid_number), + path='/v2/received-text-messages/{}'.format(invalid_number), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 400 diff --git a/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py index 9a7f654ba..6365d90a5 100644 --- a/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py +++ b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py @@ -20,7 +20,11 @@ valid_inbound_sms = { } valid_inbound_sms_list = { - "inbound_sms_list": [valid_inbound_sms] + "inbound_sms_list": [valid_inbound_sms], + "links": { + "current": valid_inbound_sms["id"] + } + } invalid_inbound_sms = { @@ -40,7 +44,7 @@ invalid_inbound_sms_list = { def test_get_inbound_sms_contract(client, sample_inbound_sms): auth_header = create_authorization_header(service_id=sample_inbound_sms.service_id) - response = client.get('/v2/inbound_sms/{}'.format(sample_inbound_sms.user_number), headers=[auth_header]) + response = client.get('/v2/received-text-messages/{}'.format(sample_inbound_sms.user_number), headers=[auth_header]) response_json = json.loads(response.get_data(as_text=True)) assert validate(response_json, get_inbound_sms_response)['inbound_sms_list'][0] \ From 2e66fe5e38c50002859a6ea57346eedb2855bb4b Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 7 Nov 2017 11:55:51 +0000 Subject: [PATCH 06/35] Removed `page` from get paginated args as not used --- app/dao/inbound_sms_dao.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 5bbc9a3a9..4813fc695 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -36,7 +36,6 @@ def dao_get_paginated_inbound_sms_for_service( service_id, user_number=None, older_than=None, - page=1, page_size=None ): if page_size is None: @@ -44,7 +43,7 @@ def dao_get_paginated_inbound_sms_for_service( filters = [InboundSms.service_id == service_id] - if older_than is not None: + if older_than: older_than_created_at = db.session.query( InboundSms.created_at).filter(InboundSms.id == older_than).as_scalar() filters.append(InboundSms.created_at < older_than_created_at) @@ -55,7 +54,6 @@ def dao_get_paginated_inbound_sms_for_service( query = InboundSms.query.filter(*filters) return query.order_by(desc(InboundSms.created_at)).paginate( - page=page, per_page=page_size ).items From 425d66bf292b3496cf40f7b7088029a030e60e1b Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 7 Nov 2017 11:56:27 +0000 Subject: [PATCH 07/35] Added more inbound sms tests --- .../v2/inbound_sms/test_get_inbound_sms.py | 94 +++++++++++++++++-- 1 file changed, 88 insertions(+), 6 deletions(-) diff --git a/tests/app/v2/inbound_sms/test_get_inbound_sms.py b/tests/app/v2/inbound_sms/test_get_inbound_sms.py index 34eb0420a..1f1f1c771 100644 --- a/tests/app/v2/inbound_sms/test_get_inbound_sms.py +++ b/tests/app/v2/inbound_sms/test_get_inbound_sms.py @@ -38,15 +38,21 @@ def test_get_all_inbound_sms_returns_200( ('v2_inbound_sms.get_all_inbound_sms', None), ('v2_inbound_sms.get_inbound_sms_by_number', '447700900111') ]) -def test_get_all_inbound_sms_generate_page_links( +def test_get_inbound_sms_generate_page_links( client, sample_service, mocker, inbound_sms_path, user_number ): - mocker.patch.dict("app.v2.inbound_sms.get_inbound_sms.current_app.config", {"API_PAGE_SIZE": 1}) + mocker.patch.dict( + "app.v2.inbound_sms.get_inbound_sms.current_app.config", + {"API_PAGE_SIZE": 2} + ) all_inbound_sms = [ create_inbound_sms(service=sample_service, user_number='447700900111', content='Hi'), create_inbound_sms(service=sample_service, user_number='447700900111'), + create_inbound_sms(service=sample_service, user_number='447700900111', content='End'), ] + reversed_inbound_sms = sorted(all_inbound_sms, key=lambda sms: sms.created_at, reverse=True) + auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( path=url_for(inbound_sms_path, user_number=user_number), @@ -55,7 +61,7 @@ def test_get_all_inbound_sms_generate_page_links( assert response.status_code == 200 json_response = json.loads(response.get_data(as_text=True)) - expected_inbound_sms_list = [all_inbound_sms[-1].serialize()] + expected_inbound_sms_list = [i.serialize() for i in reversed_inbound_sms[:2]] assert json_response['inbound_sms_list'] == expected_inbound_sms_list assert url_for( @@ -65,10 +71,80 @@ def test_get_all_inbound_sms_generate_page_links( assert url_for( inbound_sms_path, user_number=user_number, - older_than=all_inbound_sms[-1].id, + older_than=reversed_inbound_sms[1].id, _external=True) == json_response['links']['next'] +@pytest.mark.parametrize('inbound_sms_path,user_number', [ + ('v2_inbound_sms.get_all_inbound_sms', None), + ('v2_inbound_sms.get_inbound_sms_by_number', '447700900111') +]) +def test_get_next_inbound_sms_will_get_correct_inbound_sms_list( + client, sample_service, mocker, inbound_sms_path, user_number +): + mocker.patch.dict( + "app.v2.inbound_sms.get_inbound_sms.current_app.config", + {"API_PAGE_SIZE": 2} + ) + all_inbound_sms = [ + create_inbound_sms(service=sample_service, user_number='447700900111', content='1'), + create_inbound_sms(service=sample_service, user_number='447700900111', content='2'), + create_inbound_sms(service=sample_service, user_number='447700900111', content='3'), + create_inbound_sms(service=sample_service, user_number='447700900111', content='4'), + ] + reversed_inbound_sms = sorted(all_inbound_sms, key=lambda sms: sms.created_at, reverse=True) + + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.get( + path=url_for(inbound_sms_path, user_number=user_number, older_than=reversed_inbound_sms[1].id), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 200 + + json_response = json.loads(response.get_data(as_text=True)) + expected_inbound_sms_list = [i.serialize() for i in reversed_inbound_sms[2:]] + + assert json_response['inbound_sms_list'] == expected_inbound_sms_list + assert url_for( + inbound_sms_path, + user_number=user_number, + _external=True) == json_response['links']['current'] + assert url_for( + inbound_sms_path, + user_number=user_number, + older_than=reversed_inbound_sms[3].id, + _external=True) == json_response['links']['next'] + + +@pytest.mark.parametrize('inbound_sms_path,user_number', [ + ('v2_inbound_sms.get_all_inbound_sms', None), + ('v2_inbound_sms.get_inbound_sms_by_number', '447700900111') +]) +def test_get_next_inbound_sms_at_end_will_return_empty_inbound_sms_list( + client, sample_inbound_sms, mocker, inbound_sms_path, user_number +): + mocker.patch.dict( + "app.v2.inbound_sms.get_inbound_sms.current_app.config", + {"API_PAGE_SIZE": 1} + ) + + auth_header = create_authorization_header(service_id=sample_inbound_sms.service.id) + response = client.get( + path=url_for(inbound_sms_path, user_number=user_number, older_than=sample_inbound_sms.id), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 200 + + json_response = json.loads(response.get_data(as_text=True)) + expected_inbound_sms_list = [] + assert json_response['inbound_sms_list'] == expected_inbound_sms_list + assert url_for( + inbound_sms_path, + user_number=user_number, + _external=True) == json_response['links']['current'] + assert 'next' not in json_response['links'].keys() + + def test_get_all_inbound_sms_for_no_inbound_sms_returns_200( client, sample_service ): @@ -87,16 +163,22 @@ def test_get_all_inbound_sms_for_no_inbound_sms_returns_200( assert json_response == expected_response +@pytest.mark.parametrize('requested_number', [ + '447700900111', + '+447700900111', + '07700900111' +]) def test_get_inbound_sms_by_number_returns_200( - client, sample_service + client, sample_service, requested_number ): sample_inbound_sms1 = create_inbound_sms(service=sample_service, user_number='447700900111') create_inbound_sms(service=sample_service, user_number='447700900112') sample_inbound_sms2 = create_inbound_sms(service=sample_service, user_number='447700900111') + create_inbound_sms(service=sample_service, user_number='447700900113') auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path='/v2/received-text-messages/{}'.format('07700900111'), + path='/v2/received-text-messages/{}'.format(requested_number), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 From ac15e7fd8bcc74c2181b75e7b67782d905fed40e Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 7 Nov 2017 12:19:44 +0000 Subject: [PATCH 08/35] Renamed `inbound_sms_list` key name to `received_text_messages` --- app/v2/inbound_sms/get_inbound_sms.py | 4 ++-- app/v2/inbound_sms/inbound_sms_schemas.py | 4 ++-- tests/app/v2/inbound_sms/test_get_inbound_sms.py | 16 ++++++++-------- .../v2/inbound_sms/test_inbound_sms_schemas.py | 6 +++--- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/app/v2/inbound_sms/get_inbound_sms.py b/app/v2/inbound_sms/get_inbound_sms.py index 1ebae90c5..563491a55 100644 --- a/app/v2/inbound_sms/get_inbound_sms.py +++ b/app/v2/inbound_sms/get_inbound_sms.py @@ -34,7 +34,7 @@ def get_inbound_sms_by_number(user_number): ) return jsonify( - inbound_sms_list=[i.serialize() for i in paginated_inbound_sms], + received_text_messages=[i.serialize() for i in paginated_inbound_sms], links=_build_links( paginated_inbound_sms, endpoint='get_inbound_sms_by_number', @@ -58,7 +58,7 @@ def get_all_inbound_sms(): ) return jsonify( - inbound_sms_list=[i.serialize() for i in paginated_inbound_sms], + received_text_messages=[i.serialize() for i in paginated_inbound_sms], links=_build_links(paginated_inbound_sms, endpoint='get_all_inbound_sms') ), 200 diff --git a/app/v2/inbound_sms/inbound_sms_schemas.py b/app/v2/inbound_sms/inbound_sms_schemas.py index 0990c1e2e..d21c2b27b 100644 --- a/app/v2/inbound_sms/inbound_sms_schemas.py +++ b/app/v2/inbound_sms/inbound_sms_schemas.py @@ -36,7 +36,7 @@ get_inbound_sms_response = { "description": "GET list of inbound sms response schema", "type": "object", "properties": { - "inbound_sms_list": { + "received_text_messages": { "type": "array", "items": { "type": "object", @@ -57,7 +57,7 @@ get_inbound_sms_response = { "required": ["current"] } }, - "required": ["inbound_sms_list", "links"], + "required": ["received_text_messages", "links"], "definitions": { "inbound_sms": get_inbound_sms_single_response } diff --git a/tests/app/v2/inbound_sms/test_get_inbound_sms.py b/tests/app/v2/inbound_sms/test_get_inbound_sms.py index 1f1f1c771..a45a01928 100644 --- a/tests/app/v2/inbound_sms/test_get_inbound_sms.py +++ b/tests/app/v2/inbound_sms/test_get_inbound_sms.py @@ -25,7 +25,7 @@ def test_get_all_inbound_sms_returns_200( assert response.status_code == 200 assert response.headers['Content-type'] == 'application/json' - json_response = json.loads(response.get_data(as_text=True))['inbound_sms_list'] + json_response = json.loads(response.get_data(as_text=True))['received_text_messages'] reversed_all_inbound_sms = sorted(all_inbound_sms, key=lambda sms: sms.created_at, reverse=True) @@ -63,7 +63,7 @@ def test_get_inbound_sms_generate_page_links( json_response = json.loads(response.get_data(as_text=True)) expected_inbound_sms_list = [i.serialize() for i in reversed_inbound_sms[:2]] - assert json_response['inbound_sms_list'] == expected_inbound_sms_list + assert json_response['received_text_messages'] == expected_inbound_sms_list assert url_for( inbound_sms_path, user_number=user_number, @@ -104,7 +104,7 @@ def test_get_next_inbound_sms_will_get_correct_inbound_sms_list( json_response = json.loads(response.get_data(as_text=True)) expected_inbound_sms_list = [i.serialize() for i in reversed_inbound_sms[2:]] - assert json_response['inbound_sms_list'] == expected_inbound_sms_list + assert json_response['received_text_messages'] == expected_inbound_sms_list assert url_for( inbound_sms_path, user_number=user_number, @@ -137,7 +137,7 @@ def test_get_next_inbound_sms_at_end_will_return_empty_inbound_sms_list( json_response = json.loads(response.get_data(as_text=True)) expected_inbound_sms_list = [] - assert json_response['inbound_sms_list'] == expected_inbound_sms_list + assert json_response['received_text_messages'] == expected_inbound_sms_list assert url_for( inbound_sms_path, user_number=user_number, @@ -156,7 +156,7 @@ def test_get_all_inbound_sms_for_no_inbound_sms_returns_200( assert response.status_code == 200 assert response.headers['Content-type'] == 'application/json' - json_response = json.loads(response.get_data(as_text=True))['inbound_sms_list'] + json_response = json.loads(response.get_data(as_text=True))['received_text_messages'] expected_response = [] @@ -184,7 +184,7 @@ def test_get_inbound_sms_by_number_returns_200( assert response.status_code == 200 assert response.headers['Content-type'] == 'application/json' - json_response = json.loads(response.get_data(as_text=True))['inbound_sms_list'] + json_response = json.loads(response.get_data(as_text=True))['received_text_messages'] expected_response = [sample_inbound_sms2.serialize(), sample_inbound_sms1.serialize()] @@ -202,7 +202,7 @@ def test_get_inbound_sms_for_no_inbound_sms_returns_200( assert response.status_code == 200 assert response.headers['Content-type'] == 'application/json' - json_response = json.loads(response.get_data(as_text=True))['inbound_sms_list'] + json_response = json.loads(response.get_data(as_text=True))['received_text_messages'] expected_response = [] @@ -218,7 +218,7 @@ def test_get_inbound_sms_by_nonexistent_number(client, sample_service): assert response.status_code == 200 assert response.headers['Content-type'] == 'application/json' - json_response = json.loads(response.get_data(as_text=True))['inbound_sms_list'] + json_response = json.loads(response.get_data(as_text=True))['received_text_messages'] expected_response = [] assert json_response == expected_response diff --git a/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py index 6365d90a5..47db068ea 100644 --- a/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py +++ b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py @@ -20,7 +20,7 @@ valid_inbound_sms = { } valid_inbound_sms_list = { - "inbound_sms_list": [valid_inbound_sms], + "received_text_messages": [valid_inbound_sms], "links": { "current": valid_inbound_sms["id"] } @@ -38,7 +38,7 @@ invalid_inbound_sms = { } invalid_inbound_sms_list = { - "inbound_sms_list": [invalid_inbound_sms] + "received_text_messages": [invalid_inbound_sms] } @@ -47,7 +47,7 @@ def test_get_inbound_sms_contract(client, sample_inbound_sms): response = client.get('/v2/received-text-messages/{}'.format(sample_inbound_sms.user_number), headers=[auth_header]) response_json = json.loads(response.get_data(as_text=True)) - assert validate(response_json, get_inbound_sms_response)['inbound_sms_list'][0] \ + assert validate(response_json, get_inbound_sms_response)['received_text_messages'][0] \ == sample_inbound_sms.serialize() From dca331e3ea27bbeaf0fca57f33a1a26e8f5359ee Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 7 Nov 2017 16:39:11 +0000 Subject: [PATCH 09/35] Refactor to use query string rather than path params --- app/v2/inbound_sms/get_inbound_sms.py | 58 +++++------------ app/v2/inbound_sms/inbound_sms_schemas.py | 16 ++++- .../v2/inbound_sms/test_get_inbound_sms.py | 62 ++++++++++--------- .../inbound_sms/test_inbound_sms_schemas.py | 3 +- 4 files changed, 68 insertions(+), 71 deletions(-) diff --git a/app/v2/inbound_sms/get_inbound_sms.py b/app/v2/inbound_sms/get_inbound_sms.py index 563491a55..09a8c429f 100644 --- a/app/v2/inbound_sms/get_inbound_sms.py +++ b/app/v2/inbound_sms/get_inbound_sms.py @@ -1,35 +1,33 @@ -import uuid - from flask import jsonify, request, url_for, current_app -from sqlalchemy.orm.exc import NoResultFound -from werkzeug.exceptions import abort from notifications_utils.recipients import validate_and_format_phone_number from notifications_utils.recipients import InvalidPhoneError from app import authenticated_service from app.dao import inbound_sms_dao +from app.schema_validation import validate from app.v2.errors import BadRequestError from app.v2.inbound_sms import v2_inbound_sms_blueprint +from app.v2.inbound_sms.inbound_sms_schemas import get_inbound_sms_request -@v2_inbound_sms_blueprint.route("/", methods=['GET']) -def get_inbound_sms_by_number(user_number): - _data = request.args.to_dict(flat=False) +@v2_inbound_sms_blueprint.route("", methods=['GET']) +def get_inbound_sms(): + data = validate(request.args.to_dict(), get_inbound_sms_request) - # flat=False makes everything a list, but we only ever allow one value for "older_than" - if 'older_than' in _data: - _data['older_than'] = _data['older_than'][0] + if data.get('user_number'): + try: + data['user_number'] = validate_and_format_phone_number(data.get('user_number')) + except InvalidPhoneError as e: + raise BadRequestError(message=str(e)) - try: - user_number = validate_and_format_phone_number(user_number) - except InvalidPhoneError as e: - raise BadRequestError(message=str(e)) + user_number = data.get('user_number', None) + older_than = data.get('older_than', None) paginated_inbound_sms = inbound_sms_dao.dao_get_paginated_inbound_sms_for_service( authenticated_service.id, user_number=user_number, - older_than=_data.get('older_than'), + older_than=older_than, page_size=current_app.config.get('API_PAGE_SIZE') ) @@ -37,36 +35,14 @@ def get_inbound_sms_by_number(user_number): received_text_messages=[i.serialize() for i in paginated_inbound_sms], links=_build_links( paginated_inbound_sms, - endpoint='get_inbound_sms_by_number', - user_number=user_number - ) + user_number=user_number) ), 200 -@v2_inbound_sms_blueprint.route("", methods=['GET']) -def get_all_inbound_sms(): - _data = request.args.to_dict(flat=False) - - # flat=False makes everything a list, but we only ever allow one value for "older_than" - if 'older_than' in _data: - _data['older_than'] = _data['older_than'][0] - - paginated_inbound_sms = inbound_sms_dao.dao_get_paginated_inbound_sms_for_service( - authenticated_service.id, - older_than=_data.get('older_than'), - page_size=current_app.config.get('API_PAGE_SIZE') - ) - - return jsonify( - received_text_messages=[i.serialize() for i in paginated_inbound_sms], - links=_build_links(paginated_inbound_sms, endpoint='get_all_inbound_sms') - ), 200 - - -def _build_links(inbound_sms_list, endpoint, user_number=None): +def _build_links(inbound_sms_list, user_number=None): _links = { 'current': url_for( - "v2_inbound_sms.{}".format(endpoint), + "v2_inbound_sms.get_inbound_sms", user_number=user_number, _external=True, ), @@ -74,7 +50,7 @@ def _build_links(inbound_sms_list, endpoint, user_number=None): if inbound_sms_list: _links['next'] = url_for( - "v2_inbound_sms.{}".format(endpoint), + "v2_inbound_sms.get_inbound_sms", user_number=user_number, older_than=inbound_sms_list[-1].id, _external=True, diff --git a/app/v2/inbound_sms/inbound_sms_schemas.py b/app/v2/inbound_sms/inbound_sms_schemas.py index d21c2b27b..1f257cea7 100644 --- a/app/v2/inbound_sms/inbound_sms_schemas.py +++ b/app/v2/inbound_sms/inbound_sms_schemas.py @@ -1,6 +1,18 @@ from app.schema_validation.definitions import uuid +get_inbound_sms_request = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "schema for query parameters allowed when getting list of received text messages", + "type": "object", + "properties": { + "older_than": uuid, + "user_number": {"type": "string"} + }, + "additionalProperties": False, +} + + get_inbound_sms_single_response = { "$schema": "http://json-schema.org/draft-04/schema#", "description": "GET inbound sms schema response", @@ -29,6 +41,7 @@ get_inbound_sms_single_response = { "user_number", "created_at", "service_id", "notify_number", "content" ], + "additionalProperties": False, } get_inbound_sms_response = { @@ -60,5 +73,6 @@ get_inbound_sms_response = { "required": ["received_text_messages", "links"], "definitions": { "inbound_sms": get_inbound_sms_single_response - } + }, + "additionalProperties": False, } diff --git a/tests/app/v2/inbound_sms/test_get_inbound_sms.py b/tests/app/v2/inbound_sms/test_get_inbound_sms.py index a45a01928..044bc5d64 100644 --- a/tests/app/v2/inbound_sms/test_get_inbound_sms.py +++ b/tests/app/v2/inbound_sms/test_get_inbound_sms.py @@ -1,8 +1,6 @@ -import datetime import pytest from flask import json, url_for -from app import DATETIME_FORMAT from tests import create_authorization_header from tests.app.db import create_inbound_sms @@ -34,12 +32,9 @@ def test_get_all_inbound_sms_returns_200( assert json_response == expected_response -@pytest.mark.parametrize('inbound_sms_path,user_number', [ - ('v2_inbound_sms.get_all_inbound_sms', None), - ('v2_inbound_sms.get_inbound_sms_by_number', '447700900111') -]) +@pytest.mark.parametrize('user_number', [None, '447700900111']) def test_get_inbound_sms_generate_page_links( - client, sample_service, mocker, inbound_sms_path, user_number + client, sample_service, mocker, user_number ): mocker.patch.dict( "app.v2.inbound_sms.get_inbound_sms.current_app.config", @@ -55,7 +50,7 @@ def test_get_inbound_sms_generate_page_links( auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path=url_for(inbound_sms_path, user_number=user_number), + url_for('v2_inbound_sms.get_inbound_sms', user_number=user_number), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 @@ -65,22 +60,19 @@ def test_get_inbound_sms_generate_page_links( assert json_response['received_text_messages'] == expected_inbound_sms_list assert url_for( - inbound_sms_path, + 'v2_inbound_sms.get_inbound_sms', user_number=user_number, _external=True) == json_response['links']['current'] assert url_for( - inbound_sms_path, + 'v2_inbound_sms.get_inbound_sms', user_number=user_number, older_than=reversed_inbound_sms[1].id, _external=True) == json_response['links']['next'] -@pytest.mark.parametrize('inbound_sms_path,user_number', [ - ('v2_inbound_sms.get_all_inbound_sms', None), - ('v2_inbound_sms.get_inbound_sms_by_number', '447700900111') -]) +@pytest.mark.parametrize('user_number', [None, '447700900111']) def test_get_next_inbound_sms_will_get_correct_inbound_sms_list( - client, sample_service, mocker, inbound_sms_path, user_number + client, sample_service, mocker, user_number ): mocker.patch.dict( "app.v2.inbound_sms.get_inbound_sms.current_app.config", @@ -96,7 +88,7 @@ def test_get_next_inbound_sms_will_get_correct_inbound_sms_list( auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path=url_for(inbound_sms_path, user_number=user_number, older_than=reversed_inbound_sms[1].id), + path=url_for('v2_inbound_sms.get_inbound_sms', user_number=user_number, older_than=reversed_inbound_sms[1].id), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 @@ -106,22 +98,19 @@ def test_get_next_inbound_sms_will_get_correct_inbound_sms_list( assert json_response['received_text_messages'] == expected_inbound_sms_list assert url_for( - inbound_sms_path, + 'v2_inbound_sms.get_inbound_sms', user_number=user_number, _external=True) == json_response['links']['current'] assert url_for( - inbound_sms_path, + 'v2_inbound_sms.get_inbound_sms', user_number=user_number, older_than=reversed_inbound_sms[3].id, _external=True) == json_response['links']['next'] -@pytest.mark.parametrize('inbound_sms_path,user_number', [ - ('v2_inbound_sms.get_all_inbound_sms', None), - ('v2_inbound_sms.get_inbound_sms_by_number', '447700900111') -]) +@pytest.mark.parametrize('user_number', [None, '447700900111']) def test_get_next_inbound_sms_at_end_will_return_empty_inbound_sms_list( - client, sample_inbound_sms, mocker, inbound_sms_path, user_number + client, sample_inbound_sms, mocker, user_number ): mocker.patch.dict( "app.v2.inbound_sms.get_inbound_sms.current_app.config", @@ -130,7 +119,7 @@ def test_get_next_inbound_sms_at_end_will_return_empty_inbound_sms_list( auth_header = create_authorization_header(service_id=sample_inbound_sms.service.id) response = client.get( - path=url_for(inbound_sms_path, user_number=user_number, older_than=sample_inbound_sms.id), + path=url_for('v2_inbound_sms.get_inbound_sms', user_number=user_number, older_than=sample_inbound_sms.id), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 @@ -139,7 +128,7 @@ def test_get_next_inbound_sms_at_end_will_return_empty_inbound_sms_list( expected_inbound_sms_list = [] assert json_response['received_text_messages'] == expected_inbound_sms_list assert url_for( - inbound_sms_path, + 'v2_inbound_sms.get_inbound_sms', user_number=user_number, _external=True) == json_response['links']['current'] assert 'next' not in json_response['links'].keys() @@ -178,7 +167,7 @@ def test_get_inbound_sms_by_number_returns_200( auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path='/v2/received-text-messages/{}'.format(requested_number), + path='/v2/received-text-messages?user_number={}'.format(requested_number), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 @@ -209,10 +198,27 @@ def test_get_inbound_sms_for_no_inbound_sms_returns_200( assert json_response == expected_response +def test_get_inbound_sms_with_invalid_query_string_returns_400(client, sample_service): + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.get( + path='/v2/received-text-messages?usernumber=447700900000', + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 400 + assert response.headers['Content-type'] == 'application/json' + + json_response = json.loads(response.get_data(as_text=True)) + + assert json_response['status_code'] == 400 + assert json_response['errors'][0]['error'] == 'ValidationError' + assert json_response['errors'][0]['message'] == \ + 'Additional properties are not allowed (usernumber was unexpected)' + + def test_get_inbound_sms_by_nonexistent_number(client, sample_service): auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path='/v2/received-text-messages/447700900000', + path='/v2/received-text-messages?user_number=447700900000', headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 @@ -233,7 +239,7 @@ def test_get_inbound_sms_by_invalid_number( client, sample_service, invalid_number, expected_message): auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path='/v2/received-text-messages/{}'.format(invalid_number), + path='/v2/received-text-messages?user_number={}'.format(invalid_number), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 400 diff --git a/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py index 47db068ea..c445de4a6 100644 --- a/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py +++ b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py @@ -6,6 +6,7 @@ from app.v2.inbound_sms.inbound_sms_schemas import get_inbound_sms_response, get from app.schema_validation import validate from tests import create_authorization_header +from tests.app.db import create_inbound_sms valid_inbound_sms = { @@ -44,7 +45,7 @@ invalid_inbound_sms_list = { def test_get_inbound_sms_contract(client, sample_inbound_sms): auth_header = create_authorization_header(service_id=sample_inbound_sms.service_id) - response = client.get('/v2/received-text-messages/{}'.format(sample_inbound_sms.user_number), headers=[auth_header]) + response = client.get('/v2/received-text-messages', headers=[auth_header]) response_json = json.loads(response.get_data(as_text=True)) assert validate(response_json, get_inbound_sms_response)['received_text_messages'][0] \ From 874ca5a8affdaf17eaef9f93f5a4799decc090fe Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 8 Nov 2017 10:57:16 +0000 Subject: [PATCH 10/35] Refactored code to remove user number handling --- app/v2/inbound_sms/get_inbound_sms.py | 21 +-- app/v2/inbound_sms/inbound_sms_schemas.py | 1 - .../v2/inbound_sms/test_get_inbound_sms.py | 123 ++---------------- .../inbound_sms/test_inbound_sms_schemas.py | 48 +++++-- 4 files changed, 50 insertions(+), 143 deletions(-) diff --git a/app/v2/inbound_sms/get_inbound_sms.py b/app/v2/inbound_sms/get_inbound_sms.py index 09a8c429f..478e4f03d 100644 --- a/app/v2/inbound_sms/get_inbound_sms.py +++ b/app/v2/inbound_sms/get_inbound_sms.py @@ -6,7 +6,6 @@ from notifications_utils.recipients import InvalidPhoneError from app import authenticated_service from app.dao import inbound_sms_dao from app.schema_validation import validate -from app.v2.errors import BadRequestError from app.v2.inbound_sms import v2_inbound_sms_blueprint from app.v2.inbound_sms.inbound_sms_schemas import get_inbound_sms_request @@ -15,35 +14,22 @@ from app.v2.inbound_sms.inbound_sms_schemas import get_inbound_sms_request def get_inbound_sms(): data = validate(request.args.to_dict(), get_inbound_sms_request) - if data.get('user_number'): - try: - data['user_number'] = validate_and_format_phone_number(data.get('user_number')) - except InvalidPhoneError as e: - raise BadRequestError(message=str(e)) - - user_number = data.get('user_number', None) - older_than = data.get('older_than', None) - paginated_inbound_sms = inbound_sms_dao.dao_get_paginated_inbound_sms_for_service( authenticated_service.id, - user_number=user_number, - older_than=older_than, + older_than=data.get('older_than', None), page_size=current_app.config.get('API_PAGE_SIZE') ) return jsonify( received_text_messages=[i.serialize() for i in paginated_inbound_sms], - links=_build_links( - paginated_inbound_sms, - user_number=user_number) + links=_build_links(paginated_inbound_sms) ), 200 -def _build_links(inbound_sms_list, user_number=None): +def _build_links(inbound_sms_list): _links = { 'current': url_for( "v2_inbound_sms.get_inbound_sms", - user_number=user_number, _external=True, ), } @@ -51,7 +37,6 @@ def _build_links(inbound_sms_list, user_number=None): if inbound_sms_list: _links['next'] = url_for( "v2_inbound_sms.get_inbound_sms", - user_number=user_number, older_than=inbound_sms_list[-1].id, _external=True, ) diff --git a/app/v2/inbound_sms/inbound_sms_schemas.py b/app/v2/inbound_sms/inbound_sms_schemas.py index 1f257cea7..e6ef8cc98 100644 --- a/app/v2/inbound_sms/inbound_sms_schemas.py +++ b/app/v2/inbound_sms/inbound_sms_schemas.py @@ -7,7 +7,6 @@ get_inbound_sms_request = { "type": "object", "properties": { "older_than": uuid, - "user_number": {"type": "string"} }, "additionalProperties": False, } diff --git a/tests/app/v2/inbound_sms/test_get_inbound_sms.py b/tests/app/v2/inbound_sms/test_get_inbound_sms.py index 044bc5d64..e499089c3 100644 --- a/tests/app/v2/inbound_sms/test_get_inbound_sms.py +++ b/tests/app/v2/inbound_sms/test_get_inbound_sms.py @@ -1,11 +1,10 @@ -import pytest from flask import json, url_for from tests import create_authorization_header from tests.app.db import create_inbound_sms -def test_get_all_inbound_sms_returns_200( +def test_get_inbound_sms_returns_200( client, sample_service ): all_inbound_sms = [ @@ -32,10 +31,7 @@ def test_get_all_inbound_sms_returns_200( assert json_response == expected_response -@pytest.mark.parametrize('user_number', [None, '447700900111']) -def test_get_inbound_sms_generate_page_links( - client, sample_service, mocker, user_number -): +def test_get_inbound_sms_generate_page_links(client, sample_service, mocker): mocker.patch.dict( "app.v2.inbound_sms.get_inbound_sms.current_app.config", {"API_PAGE_SIZE": 2} @@ -50,7 +46,7 @@ def test_get_inbound_sms_generate_page_links( auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - url_for('v2_inbound_sms.get_inbound_sms', user_number=user_number), + url_for('v2_inbound_sms.get_inbound_sms'), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 @@ -61,19 +57,14 @@ def test_get_inbound_sms_generate_page_links( assert json_response['received_text_messages'] == expected_inbound_sms_list assert url_for( 'v2_inbound_sms.get_inbound_sms', - user_number=user_number, _external=True) == json_response['links']['current'] assert url_for( 'v2_inbound_sms.get_inbound_sms', - user_number=user_number, older_than=reversed_inbound_sms[1].id, _external=True) == json_response['links']['next'] -@pytest.mark.parametrize('user_number', [None, '447700900111']) -def test_get_next_inbound_sms_will_get_correct_inbound_sms_list( - client, sample_service, mocker, user_number -): +def test_get_next_inbound_sms_will_get_correct_inbound_sms_list(client, sample_service, mocker): mocker.patch.dict( "app.v2.inbound_sms.get_inbound_sms.current_app.config", {"API_PAGE_SIZE": 2} @@ -88,7 +79,7 @@ def test_get_next_inbound_sms_will_get_correct_inbound_sms_list( auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path=url_for('v2_inbound_sms.get_inbound_sms', user_number=user_number, older_than=reversed_inbound_sms[1].id), + path=url_for('v2_inbound_sms.get_inbound_sms', older_than=reversed_inbound_sms[1].id), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 @@ -99,19 +90,14 @@ def test_get_next_inbound_sms_will_get_correct_inbound_sms_list( assert json_response['received_text_messages'] == expected_inbound_sms_list assert url_for( 'v2_inbound_sms.get_inbound_sms', - user_number=user_number, _external=True) == json_response['links']['current'] assert url_for( 'v2_inbound_sms.get_inbound_sms', - user_number=user_number, older_than=reversed_inbound_sms[3].id, _external=True) == json_response['links']['next'] -@pytest.mark.parametrize('user_number', [None, '447700900111']) -def test_get_next_inbound_sms_at_end_will_return_empty_inbound_sms_list( - client, sample_inbound_sms, mocker, user_number -): +def test_get_next_inbound_sms_at_end_will_return_empty_inbound_sms_list(client, sample_inbound_sms, mocker): mocker.patch.dict( "app.v2.inbound_sms.get_inbound_sms.current_app.config", {"API_PAGE_SIZE": 1} @@ -119,7 +105,7 @@ def test_get_next_inbound_sms_at_end_will_return_empty_inbound_sms_list( auth_header = create_authorization_header(service_id=sample_inbound_sms.service.id) response = client.get( - path=url_for('v2_inbound_sms.get_inbound_sms', user_number=user_number, older_than=sample_inbound_sms.id), + path=url_for('v2_inbound_sms.get_inbound_sms', older_than=sample_inbound_sms.id), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 200 @@ -129,58 +115,11 @@ def test_get_next_inbound_sms_at_end_will_return_empty_inbound_sms_list( assert json_response['received_text_messages'] == expected_inbound_sms_list assert url_for( 'v2_inbound_sms.get_inbound_sms', - user_number=user_number, _external=True) == json_response['links']['current'] assert 'next' not in json_response['links'].keys() -def test_get_all_inbound_sms_for_no_inbound_sms_returns_200( - client, sample_service -): - auth_header = create_authorization_header(service_id=sample_service.id) - response = client.get( - path='/v2/received-text-messages', - 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))['received_text_messages'] - - expected_response = [] - - assert json_response == expected_response - - -@pytest.mark.parametrize('requested_number', [ - '447700900111', - '+447700900111', - '07700900111' -]) -def test_get_inbound_sms_by_number_returns_200( - client, sample_service, requested_number -): - sample_inbound_sms1 = create_inbound_sms(service=sample_service, user_number='447700900111') - create_inbound_sms(service=sample_service, user_number='447700900112') - sample_inbound_sms2 = create_inbound_sms(service=sample_service, user_number='447700900111') - create_inbound_sms(service=sample_service, user_number='447700900113') - - auth_header = create_authorization_header(service_id=sample_service.id) - response = client.get( - path='/v2/received-text-messages?user_number={}'.format(requested_number), - 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))['received_text_messages'] - - expected_response = [sample_inbound_sms2.serialize(), sample_inbound_sms1.serialize()] - - assert json_response == expected_response - - -def test_get_inbound_sms_for_no_inbound_sms_returns_200( +def test_get_inbound_sms_for_no_inbound_sms_returns_empty_list( client, sample_service ): auth_header = create_authorization_header(service_id=sample_service.id) @@ -201,7 +140,7 @@ def test_get_inbound_sms_for_no_inbound_sms_returns_200( def test_get_inbound_sms_with_invalid_query_string_returns_400(client, sample_service): auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( - path='/v2/received-text-messages?usernumber=447700900000', + path='/v2/received-text-messages?user_number=447700900000', headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 400 @@ -212,46 +151,4 @@ def test_get_inbound_sms_with_invalid_query_string_returns_400(client, sample_se assert json_response['status_code'] == 400 assert json_response['errors'][0]['error'] == 'ValidationError' assert json_response['errors'][0]['message'] == \ - 'Additional properties are not allowed (usernumber was unexpected)' - - -def test_get_inbound_sms_by_nonexistent_number(client, sample_service): - auth_header = create_authorization_header(service_id=sample_service.id) - response = client.get( - path='/v2/received-text-messages?user_number=447700900000', - 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))['received_text_messages'] - expected_response = [] - - assert json_response == expected_response - - -@pytest.mark.parametrize('invalid_number,expected_message', [ - ('0077700', 'Not enough digits'), - ('123456789012', 'Not a UK mobile number'), - ('invalid_number', 'Must not contain letters or symbols') -]) -def test_get_inbound_sms_by_invalid_number( - client, sample_service, invalid_number, expected_message): - auth_header = create_authorization_header(service_id=sample_service.id) - response = client.get( - path='/v2/received-text-messages?user_number={}'.format(invalid_number), - headers=[('Content-Type', 'application/json'), auth_header]) - - assert response.status_code == 400 - assert response.headers['Content-type'] == 'application/json' - - json_response = json.loads(response.get_data(as_text=True)) - assert json_response == { - "errors": [ - { - "error": "BadRequestError", - "message": expected_message - } - ], - "status_code": 400 - } + 'Additional properties are not allowed (user_number was unexpected)' diff --git a/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py index c445de4a6..fe9613056 100644 --- a/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py +++ b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py @@ -1,14 +1,17 @@ import pytest -from flask import json +from flask import json, url_for from jsonschema.exceptions import ValidationError -from app.v2.inbound_sms.inbound_sms_schemas import get_inbound_sms_response, get_inbound_sms_single_response +from app.v2.inbound_sms.inbound_sms_schemas import ( + get_inbound_sms_request, + get_inbound_sms_response, + get_inbound_sms_single_response +) from app.schema_validation import validate from tests import create_authorization_header from tests.app.db import create_inbound_sms - valid_inbound_sms = { "provider_date": "2017-11-02T15:07:57.199541Z", "provider_reference": "foo", @@ -43,28 +46,51 @@ invalid_inbound_sms_list = { } -def test_get_inbound_sms_contract(client, sample_inbound_sms): - auth_header = create_authorization_header(service_id=sample_inbound_sms.service_id) +def test_get_inbound_sms_contract(client, sample_service): + all_inbound_sms = [ + create_inbound_sms(service=sample_service, user_number='447700900113'), + create_inbound_sms(service=sample_service, user_number='447700900112'), + create_inbound_sms(service=sample_service, user_number='447700900111'), + ] + reversed_inbound_sms = sorted(all_inbound_sms, key=lambda sms: sms.created_at, reverse=True) + + auth_header = create_authorization_header(service_id=all_inbound_sms[0].service_id) response = client.get('/v2/received-text-messages', headers=[auth_header]) response_json = json.loads(response.get_data(as_text=True)) - assert validate(response_json, get_inbound_sms_response)['received_text_messages'][0] \ - == sample_inbound_sms.serialize() + validated_resp = validate(response_json, get_inbound_sms_response) + assert validated_resp['received_text_messages'] == [i.serialize() for i in reversed_inbound_sms] + assert validated_resp['links']['current'] == url_for( + 'v2_inbound_sms.get_inbound_sms', _external=True) + assert validated_resp['links']['next'] == url_for( + 'v2_inbound_sms.get_inbound_sms', older_than=all_inbound_sms[0].id, _external=True) -def test_valid_inbound_sms_json(): +@pytest.mark.parametrize('request_args', [ + {'older_than': "6ce466d0-fd6a-11e5-82f5-e0accb9d11a6"}, {}] +) +def test_valid_inbound_sms_request_json(client, request_args): + validate(request_args, get_inbound_sms_request) + + +def test_invalid_inbound_sms_request_json(client): + with pytest.raises(expected_exception=ValidationError): + validate({'user_number': '447700900111'}, get_inbound_sms_request) + + +def test_valid_inbound_sms_response_json(): assert validate(valid_inbound_sms, get_inbound_sms_single_response) == valid_inbound_sms -def test_valid_inbound_sms_list_json(): +def test_valid_inbound_sms_list_response_json(): validate(valid_inbound_sms_list, get_inbound_sms_response) -def test_invalid_inbound_sms_json(): +def test_invalid_inbound_sms_response_json(): with pytest.raises(expected_exception=ValidationError): validate(invalid_inbound_sms, get_inbound_sms_single_response) -def test_invalid_inbound_sms_list_json(): +def test_invalid_inbound_sms_list_response_json(): with pytest.raises(expected_exception=ValidationError): validate(invalid_inbound_sms_list, get_inbound_sms_response) From 4761d852e9fa785a1f049efcd0777c7ae5a2ef75 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 8 Nov 2017 13:32:30 +0000 Subject: [PATCH 11/35] Fix escaping in inbound text messages from MMG MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One of our providers gives us messages with special characters escaped, ie a newline comes through as `\n`, not a literal newline. We shouldn’t be showing these backslashes to any of our users. We also have examples of real inbound messages containing `👍` and `’`, so we should continue to display these properly. It’s a bit tricky, because the strings we get from this provider are a mixture of escape sequences (eg `\n`) and unicode characters (eg `😨`). So we have to first convert the unicode character `😨` into an escape sequence, `\U0001f628` in this example. We do this by encoding with the `raw_unicode_escape` codec: > Latin-1 encoding with \uXXXX and \UXXXXXXXX for other code points. > Existing backslashes are not escaped in any way. It is used in the > Python pickle protocol. – https://docs.python.org/3/library/codecs.html#text-encodings Then we turn this back into a string using the `unicode_escape` codec, which transforms all escape sequences into their literal representations (eg `\U0001f628` becomes `😨` and `\n` becomes a newline). --- app/notifications/receive_notifications.py | 6 +++- .../test_receive_notification.py | 31 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/app/notifications/receive_notifications.py b/app/notifications/receive_notifications.py index ac116b28a..fa9d11146 100644 --- a/app/notifications/receive_notifications.py +++ b/app/notifications/receive_notifications.py @@ -82,7 +82,11 @@ def receive_firetext_sms(): def format_mmg_message(message): - return unquote(message.replace('+', ' ')) + return unescape_string(unquote(message.replace('+', ' '))) + + +def unescape_string(string): + return string.encode('raw_unicode_escape').decode('unicode_escape') def format_mmg_datetime(date): diff --git a/tests/app/notifications/test_receive_notification.py b/tests/app/notifications/test_receive_notification.py index 26a628bb7..d5d504099 100644 --- a/tests/app/notifications/test_receive_notification.py +++ b/tests/app/notifications/test_receive_notification.py @@ -12,6 +12,7 @@ from app.notifications.receive_notifications import ( create_inbound_sms_object, strip_leading_forty_four, has_inbound_sms_permissions, + unescape_string, ) from app.models import InboundSms, EMAIL_TYPE, SMS_TYPE, INBOUND_SMS_TYPE @@ -166,6 +167,36 @@ def test_format_mmg_message(message, expected_output): assert format_mmg_message(message) == expected_output +@pytest.mark.parametrize('raw, expected', [ + ( + '😬', + '😬', + ), + ( + '1\\n2', + '1\n2', + ), + ( + '\\\'"\\\'', + '\'"\'', + ), + ( + """ + + """, + """ + + """, + ), + ( + '\x79 \\x79 \\\\x79', # we should never see the middle one + 'y y \\x79', + ), +]) +def test_unescape_string(raw, expected): + assert unescape_string(raw) == expected + + @pytest.mark.parametrize('provider_date, expected_output', [ ('2017-01-21+11%3A56%3A11', datetime(2017, 1, 21, 11, 56, 11)), ('2017-05-21+11%3A56%3A11', datetime(2017, 5, 21, 10, 56, 11)) From a77c316d2f7a7afae744e5ad6d9194f93e53588a Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Fri, 22 Sep 2017 12:22:40 +0100 Subject: [PATCH 12/35] Added stats annotation to a few more methods to track which methods need to be improved to increase billing performance --- app/dao/monthly_billing_dao.py | 2 ++ app/dao/notification_usage_dao.py | 1 + 2 files changed, 3 insertions(+) diff --git a/app/dao/monthly_billing_dao.py b/app/dao/monthly_billing_dao.py index 606c02cc9..4528f414d 100644 --- a/app/dao/monthly_billing_dao.py +++ b/app/dao/monthly_billing_dao.py @@ -26,6 +26,7 @@ def get_service_ids_that_need_billing_populated(start_date, end_date): ).distinct().all() +@statsd(namespace="dao") def create_or_update_monthly_billing(service_id, billing_month): start_date, end_date = get_month_start_and_end_date_in_utc(billing_month) _update_monthly_billing(service_id, start_date, end_date, SMS_TYPE) @@ -47,6 +48,7 @@ def _monthly_billing_data_to_json(billing_data): return results +@statsd(namespace="dao") @transactional def _update_monthly_billing(service_id, start_date, end_date, notification_type): billing_data = get_billing_data_for_month( diff --git a/app/dao/notification_usage_dao.py b/app/dao/notification_usage_dao.py index 7542fb0df..afb547287 100644 --- a/app/dao/notification_usage_dao.py +++ b/app/dao/notification_usage_dao.py @@ -105,6 +105,7 @@ def is_between(date, start_date, end_date): return start_date <= date <= end_date +@statsd(namespace="dao") def billing_data_per_month_query(rate, service_id, start_date, end_date, notification_type): month = get_london_month_from_utc_column(NotificationHistory.created_at) if notification_type == SMS_TYPE: From 8973e42b8602fe2dd1153bda0cfc32abc8552f40 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 7 Nov 2017 14:50:05 +0000 Subject: [PATCH 13/35] =?UTF-8?q?Added=20a=20new=20table=20=E2=80=98stats?= =?UTF-8?q?=5Ftemplate=5Fusage=5Fby=5Fmonth=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently the 'See templates used by month' report is timing out for some services due to the query time of the data from notification_history This is a stats table which will hold the data and will be updated by a scheduled celery task once a day. This data will be combined with the 'live' data from notifications tables (which will be considerably less) to form the data of the new report. --- app/models.py | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/app/models.py b/app/models.py index 74b0af79a..0c64a4581 100644 --- a/app/models.py +++ b/app/models.py @@ -1555,3 +1555,37 @@ class AuthType(db.Model): __tablename__ = 'auth_type' name = db.Column(db.String, primary_key=True) + + +class StatsTemplateUsageByMonth(db.Model): + __tablename__ = "stats_template_usage_by_month" + + template_id = db.Column( + UUID(as_uuid=True), + db.ForeignKey('templates.id'), + unique=False, + index=True, + nullable=False, + primary_key=True + ) + month = db.Column( + db.Integer, + nullable=False, + index=True, + unique=False, + primary_key=True, + default=datetime.datetime.month + ) + year = db.Column( + db.Integer, + nullable=False, + index=True, + unique=False, + primary_key=True, + default=datetime.datetime.year + ) + count = db.Column( + db.Integer, + nullable=False, + default=0 + ) From 26617a921e37d58d74f31d32339c1878297b19ba Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 7 Nov 2017 14:57:31 +0000 Subject: [PATCH 14/35] Added database migration file Currently the 'See templates used by month' report is timing out for some services due to the query time of the data from notification_history This is a stats table which will hold the data and will be updated by a scheduled celery task once a day. This data will be combined with the 'live' data from notifications tables (which will be considerably less) to form the data of the new report. --- .../versions/0134_stats_template_usage.py | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 migrations/versions/0134_stats_template_usage.py diff --git a/migrations/versions/0134_stats_template_usage.py b/migrations/versions/0134_stats_template_usage.py new file mode 100644 index 000000000..7e1b26837 --- /dev/null +++ b/migrations/versions/0134_stats_template_usage.py @@ -0,0 +1,65 @@ +""" + +Revision ID: 0134_stats_template_usage +Revises: 0133_set_services_sms_prefix +Create Date: 2017-11-07 14:35:04.798561 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0134_stats_template_usage' +down_revision = '0133_set_services_sms_prefix' + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('stats_template_usage_by_month', + sa.Column('template_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('month', sa.Integer(), nullable=False), + sa.Column('year', sa.Integer(), nullable=False), + sa.Column('count', sa.Integer(), nullable=False), + sa.ForeignKeyConstraint(['template_id'], ['templates.id'], ), + sa.PrimaryKeyConstraint('template_id', 'month', 'year') + ) + op.create_index(op.f('ix_stats_template_usage_by_month_month'), 'stats_template_usage_by_month', ['month'], unique=False) + op.create_index(op.f('ix_stats_template_usage_by_month_template_id'), 'stats_template_usage_by_month', ['template_id'], unique=False) + op.create_index(op.f('ix_stats_template_usage_by_month_year'), 'stats_template_usage_by_month', ['year'], unique=False) + op.drop_table('notification_statistics') + op.alter_column('notification_history', 'international', + existing_type=sa.BOOLEAN(), + nullable=False) + op.alter_column('notifications', 'international', + existing_type=sa.BOOLEAN(), + nullable=False) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column('notifications', 'international', + existing_type=sa.BOOLEAN(), + nullable=True) + op.alter_column('notification_history', 'international', + existing_type=sa.BOOLEAN(), + nullable=True) + op.create_table('notification_statistics', + sa.Column('id', postgresql.UUID(), autoincrement=False, nullable=False), + sa.Column('service_id', postgresql.UUID(), autoincrement=False, nullable=False), + sa.Column('emails_requested', sa.BIGINT(), autoincrement=False, nullable=False), + sa.Column('emails_delivered', sa.BIGINT(), autoincrement=False, nullable=False), + sa.Column('emails_failed', sa.BIGINT(), autoincrement=False, nullable=False), + sa.Column('sms_requested', sa.BIGINT(), autoincrement=False, nullable=False), + sa.Column('sms_delivered', sa.BIGINT(), autoincrement=False, nullable=False), + sa.Column('sms_failed', sa.BIGINT(), autoincrement=False, nullable=False), + sa.Column('day', sa.DATE(), autoincrement=False, nullable=False), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], name='notification_statistics_service_id_fkey'), + sa.PrimaryKeyConstraint('id', name='notification_statistics_pkey'), + sa.UniqueConstraint('service_id', 'day', name='uix_service_to_day') + ) + op.drop_index(op.f('ix_stats_template_usage_by_month_year'), table_name='stats_template_usage_by_month') + op.drop_index(op.f('ix_stats_template_usage_by_month_template_id'), table_name='stats_template_usage_by_month') + op.drop_index(op.f('ix_stats_template_usage_by_month_month'), table_name='stats_template_usage_by_month') + op.drop_table('stats_template_usage_by_month') + # ### end Alembic commands ### From ab43803453682eb734caf4b7c88f2b2858ad965f Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Tue, 7 Nov 2017 15:30:08 +0000 Subject: [PATCH 15/35] Removed unwanted migration commands There were some unwanted db migration commands which crept into the migration file. removed these as they are being handled in other PRs. --- migrations/versions/0134_stats_template_usage.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/migrations/versions/0134_stats_template_usage.py b/migrations/versions/0134_stats_template_usage.py index 7e1b26837..4a1fd1963 100644 --- a/migrations/versions/0134_stats_template_usage.py +++ b/migrations/versions/0134_stats_template_usage.py @@ -26,24 +26,11 @@ def upgrade(): op.create_index(op.f('ix_stats_template_usage_by_month_month'), 'stats_template_usage_by_month', ['month'], unique=False) op.create_index(op.f('ix_stats_template_usage_by_month_template_id'), 'stats_template_usage_by_month', ['template_id'], unique=False) op.create_index(op.f('ix_stats_template_usage_by_month_year'), 'stats_template_usage_by_month', ['year'], unique=False) - op.drop_table('notification_statistics') - op.alter_column('notification_history', 'international', - existing_type=sa.BOOLEAN(), - nullable=False) - op.alter_column('notifications', 'international', - existing_type=sa.BOOLEAN(), - nullable=False) # ### end Alembic commands ### def downgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.alter_column('notifications', 'international', - existing_type=sa.BOOLEAN(), - nullable=True) - op.alter_column('notification_history', 'international', - existing_type=sa.BOOLEAN(), - nullable=True) op.create_table('notification_statistics', sa.Column('id', postgresql.UUID(), autoincrement=False, nullable=False), sa.Column('service_id', postgresql.UUID(), autoincrement=False, nullable=False), From ff911c30d67f5db388f31f6305de3f0a75a42e9f Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Thu, 9 Nov 2017 10:32:39 +0000 Subject: [PATCH 16/35] Added Scheduled task to get stats for template usage Currently some pages are timing out due to the time it takes to perform database queries. This is an attempt to improve the performance by performing the query against the notification history table once a day and use the notification table for a delta between midnight and the when the page is run and combine the results. - Added Celery task for doing the work - Added a dao to handle the insert and update of the stats table - Updated tests to test the new functionality --- app/celery/scheduled_tasks.py | 18 +++ app/config.py | 5 + app/dao/services_dao.py | 28 +++- app/dao/stats_template_usage_by_month_dao.py | 24 ++++ app/utils.py | 16 +++ tests/app/celery/test_scheduled_tasks.py | 124 +++++++++++++++++- tests/app/dao/test_services_dao.py | 39 +++++- .../test_stats_template_usage_by_month_dao.py | 45 +++++++ 8 files changed, 288 insertions(+), 11 deletions(-) create mode 100644 app/dao/stats_template_usage_by_month_dao.py create mode 100644 tests/app/dao/test_stats_template_usage_by_month_dao.py diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 2e0413fe8..9b1a830d9 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -11,6 +11,10 @@ from notifications_utils.s3 import s3upload from app.aws import s3 from app import notify_celery +from app.dao.services_dao import ( + dao_fetch_monthly_historical_stats_by_template +) +from app.dao.stats_template_usage_by_month_dao import insert_or_update_stats_for_template from app.performance_platform import total_sent_notifications, processing_time from app import performance_platform_client from app.dao.date_util import get_month_start_and_end_date_in_utc @@ -402,3 +406,17 @@ def check_job_status(): queue=QueueNames.JOBS ) raise JobIncompleteError("Job(s) {} have not completed.".format(job_ids)) + + +@notify_celery.task(name='daily-stats-template_usage_by_month') +@statsd(namespace="tasks") +def daily_stats_template_usage_my_month(): + results = dao_fetch_monthly_historical_stats_by_template() + + for result in results: + insert_or_update_stats_for_template( + result.template_id, + result.month.month, + result.year.year, + result.count + ) diff --git a/app/config.py b/app/config.py index 59be8ad02..afd70dade 100644 --- a/app/config.py +++ b/app/config.py @@ -241,6 +241,11 @@ class Config(object): 'task': 'check-job-status', 'schedule': crontab(), 'options': {'queue': QueueNames.PERIODIC} + }, + 'daily-stats-template_usage_by_month': { + 'task': 'daily-stats-template_usage_by_month', + 'schedule': crontab(hour=00, minute=50), + 'options': {'queue': QueueNames.PERIODIC} } } CELERY_QUEUES = [] diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 93905e7e6..684696d83 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -1,7 +1,7 @@ import uuid -from datetime import date, datetime, timedelta +from datetime import date, datetime, timedelta, time -from sqlalchemy import asc, func +from sqlalchemy import asc, func, extract from sqlalchemy.orm import joinedload from flask import current_app @@ -40,7 +40,7 @@ from app.models import ( ) from app.service.statistics import format_monthly_template_notification_stats from app.statsd_decorators import statsd -from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc +from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc, get_london_year_from_utc_column from app.dao.annual_billing_dao import dao_insert_annual_billing DEFAULT_SERVICE_PERMISSIONS = [ @@ -520,3 +520,25 @@ def dao_fetch_active_users_for_service(service_id): ) return query.all() + + +@statsd(namespace="dao") +def dao_fetch_monthly_historical_stats_by_template(): + month = get_london_month_from_utc_column(NotificationHistory.created_at) + year = get_london_year_from_utc_column(NotificationHistory.created_at) + end_date = datetime.combine(date.today(), time.min) + + return db.session.query( + NotificationHistory.template_id, + month.label('month'), + year.label('year'), + func.count().label('count') + ).filter( + NotificationHistory.created_at < end_date + ).group_by( + NotificationHistory.template_id, + month, + year + ).order_by( + NotificationHistory.template_id + ).all() diff --git a/app/dao/stats_template_usage_by_month_dao.py b/app/dao/stats_template_usage_by_month_dao.py new file mode 100644 index 000000000..ca94dd83f --- /dev/null +++ b/app/dao/stats_template_usage_by_month_dao.py @@ -0,0 +1,24 @@ +from app import db +from app.models import StatsTemplateUsageByMonth + + +def insert_or_update_stats_for_template(template_id, month, year, count): + result = db.session.query( + StatsTemplateUsageByMonth + ).filter( + StatsTemplateUsageByMonth.template_id == template_id, + StatsTemplateUsageByMonth.month == month, + StatsTemplateUsageByMonth.year == year + ).update( + { + 'count': count + } + ) + if result == 0: + new_sms_sender = StatsTemplateUsageByMonth( + template_id=template_id, + month=month, + year=year, + count=count + ) + db.session.add(new_sms_sender) diff --git a/app/utils.py b/app/utils.py index 49f6d61dc..7fe4bdf89 100644 --- a/app/utils.py +++ b/app/utils.py @@ -75,6 +75,22 @@ def get_london_month_from_utc_column(column): ) +def get_london_year_from_utc_column(column): + """ + Where queries need to count notifications by month it needs to be + the month in BST (British Summer Time). + The database stores all timestamps as UTC without the timezone. + - First set the timezone on created_at to UTC + - then convert the timezone to BST (or Europe/London) + - lastly truncate the datetime to month with which we can group + queries + """ + return func.date_trunc( + "month", + func.timezone("Europe/London", func.timezone("UTC", column)) + ) + + def cache_key_for_service_template_counter(service_id, limit_days=7): return "{}-template-counter-limit-{}-days".format(service_id, limit_days) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index a61c70807..7f60318cc 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -2,11 +2,13 @@ from datetime import datetime, timedelta from functools import partial from unittest.mock import call, patch, PropertyMock +import functools from flask import current_app import pytest from freezegun import freeze_time +from app import db from app.celery import scheduled_tasks from app.celery.scheduled_tasks import ( check_job_status, @@ -30,8 +32,8 @@ from app.celery.scheduled_tasks import ( send_total_sent_notifications_to_performance_platform, switch_current_sms_provider_on_slow_delivery, timeout_job_statistics, - timeout_notifications -) + timeout_notifications, + daily_stats_template_usage_my_month) from app.clients.performance_platform.performance_platform_client import PerformancePlatformClient from app.config import QueueNames, TaskNames from app.dao.jobs_dao import dao_get_job_by_id @@ -49,19 +51,24 @@ from app.models import ( NOTIFICATION_PENDING, NOTIFICATION_CREATED, KEY_TYPE_TEST, - MonthlyBilling -) + MonthlyBilling, + StatsTemplateUsageByMonth) from app.utils import get_london_midnight_in_utc from app.v2.errors import JobIncompleteError from tests.app.db import create_notification, create_service, create_template, create_job, create_rate from tests.app.conftest import ( sample_job as create_sample_job, - sample_notification_history as create_notification_history, create_custom_template, datetime_in_past ) from tests.app.aws.test_s3 import single_s3_object_stub -from tests.conftest import set_config_values +from tests.conftest import set_config_values, notify_db, notify_db_session + +from tests.app.conftest import ( + sample_notification as create_notification, + sample_notification_history as create_notification_history, + sample_template as create_sample_template +) def _create_slow_delivery_notification(provider='mmg'): @@ -834,3 +841,108 @@ def test_check_job_status_task_raises_job_incomplete_error_for_multiple_jobs(moc args=([str(job.id), str(job_2.id)],), queue=QueueNames.JOBS ) + + +def test_daily_stats_template_usage_my_month(notify_db, notify_db_session): + notification_history = functools.partial( + create_notification_history, + notify_db, + notify_db_session, + status='delivered' + ) + + template_one = create_sample_template(notify_db, notify_db_session) + template_two = create_sample_template(notify_db, notify_db_session) + + notification_history(created_at=datetime(2017, 10, 1), sample_template=template_one) + notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) + notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) + notification_history(created_at=datetime.now(), sample_template=template_two) + + daily_stats_template_usage_my_month() + + results = db.session.query(StatsTemplateUsageByMonth).all() + + assert len(results) == 2 + + for result in results: + if result.template_id == template_one.id: + assert result.template_id == template_one.id + assert result.month == 10 + assert result.year == 2017 + assert result.count == 1 + elif result.template_id == template_two.id: + assert result.template_id == template_two.id + assert result.month == 4 + assert result.year == 2016 + assert result.count == 2 + else: + raise AssertionError() + + +def test_daily_stats_template_usage_my_month_no_data(): + daily_stats_template_usage_my_month() + + results = db.session.query(StatsTemplateUsageByMonth).all() + + assert len(results) == 0 + + +def test_daily_stats_template_usage_my_month_multiple_runs(notify_db, notify_db_session): + notification_history = functools.partial( + create_notification_history, + notify_db, + notify_db_session, + status='delivered' + ) + + template_one = create_sample_template(notify_db, notify_db_session) + template_two = create_sample_template(notify_db, notify_db_session) + + notification_history(created_at=datetime(2017, 10, 1), sample_template=template_one) + notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) + notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) + notification_history(created_at=datetime.now(), sample_template=template_two) + + daily_stats_template_usage_my_month() + + template_three = create_sample_template(notify_db, notify_db_session) + + notification_history(created_at=datetime(2017, 10, 1), sample_template=template_three) + notification_history(created_at=datetime(2017, 9, 1), sample_template=template_three) + notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) + notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) + notification_history(created_at=datetime.now(), sample_template=template_two) + + daily_stats_template_usage_my_month() + + results = db.session.query(StatsTemplateUsageByMonth).all() + + assert len(results) == 4 + + for result in results: + if result.template_id == template_one.id: + assert result.template_id == template_one.id + assert result.month == 10 + assert result.year == 2017 + assert result.count == 1 + elif result.template_id == template_two.id: + assert result.template_id == template_two.id + assert result.month == 4 + assert result.year == 2016 + assert result.count == 4 + elif result.template_id == template_three.id: + if result.month == 10: + assert result.template_id == template_three.id + assert result.month == 10 + assert result.year == 2017 + assert result.count == 1 + elif result.month == 9: + assert result.template_id == template_three.id + assert result.month == 9 + assert result.year == 2017 + assert result.count == 1 + else: + raise AssertionError() + else: + raise AssertionError() diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index c45ce0272..bc5a8af82 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -30,8 +30,8 @@ from app.dao.services_dao import ( dao_suspend_service, dao_resume_service, dao_fetch_active_users_for_service, - dao_fetch_service_by_inbound_number -) + dao_fetch_service_by_inbound_number, + dao_fetch_monthly_historical_stats_by_template_for_service_without_status) from app.dao.service_permissions_dao import dao_add_service_permission, dao_remove_service_permission from app.dao.users_dao import save_model_user from app.models import ( @@ -1004,3 +1004,38 @@ def _assert_service_permissions(service_permissions, expected): assert len(service_permissions) == len(expected) assert set(expected) == set(p.permission for p in service_permissions) + + +def test_dao_fetch_monthly_historical_stats_by_template(notify_db, notify_db_session): + notification_history = functools.partial( + create_notification_history, + notify_db, + notify_db_session, + status='delivered' + ) + + template_one = create_sample_template(notify_db, notify_db_session) + template_two = create_sample_template(notify_db, notify_db_session) + + notification_history(created_at=datetime(2017, 10, 1), sample_template=template_one) + notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) + notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) + notification_history(created_at=datetime.now(), sample_template=template_two) + + results = dao_fetch_monthly_historical_stats_by_template_for_service_without_status() + + assert len(results) == 2 + + for result in results: + if result.template_id == template_one.id: + assert result.template_id == template_one.id + assert result.month.month == 10 + assert result.year.year == 2017 + assert result.count == 1 + elif result.template_id == template_two.id: + assert result.template_id == template_two.id + assert result.month.month == 4 + assert result.year.year == 2016 + assert result.count == 2 + else: + raise AssertionError() diff --git a/tests/app/dao/test_stats_template_usage_by_month_dao.py b/tests/app/dao/test_stats_template_usage_by_month_dao.py new file mode 100644 index 000000000..ae46c682b --- /dev/null +++ b/tests/app/dao/test_stats_template_usage_by_month_dao.py @@ -0,0 +1,45 @@ +import pytest + +from app.dao.stats_template_usage_by_month_dao import insert_or_update_stats_for_template +from app.models import StatsTemplateUsageByMonth + +from tests.app.conftest import sample_notification, sample_email_template, sample_template, sample_job, sample_service + + +def test_create_stats_for_template(notify_db_session, sample_template): + assert StatsTemplateUsageByMonth.query.count() == 0 + + insert_or_update_stats_for_template(sample_template.id, 1, 2017, 10) + stats_by_month = StatsTemplateUsageByMonth.query.filter( + StatsTemplateUsageByMonth.template_id == sample_template.id + ).all() + + assert len(stats_by_month) == 1 + assert stats_by_month[0].template_id == sample_template.id + assert stats_by_month[0].month == 1 + assert stats_by_month[0].year == 2017 + assert stats_by_month[0].count == 10 + + +def test_update_stats_for_template(notify_db_session, sample_template): + assert StatsTemplateUsageByMonth.query.count() == 0 + + insert_or_update_stats_for_template(sample_template.id, 1, 2017, 10) + insert_or_update_stats_for_template(sample_template.id, 1, 2017, 20) + insert_or_update_stats_for_template(sample_template.id, 2, 2017, 30) + + stats_by_month = StatsTemplateUsageByMonth.query.filter( + StatsTemplateUsageByMonth.template_id == sample_template.id + ).order_by(StatsTemplateUsageByMonth.template_id).all() + + assert len(stats_by_month) == 2 + + assert stats_by_month[0].template_id == sample_template.id + assert stats_by_month[0].month == 1 + assert stats_by_month[0].year == 2017 + assert stats_by_month[0].count == 20 + + assert stats_by_month[1].template_id == sample_template.id + assert stats_by_month[1].month == 2 + assert stats_by_month[1].year == 2017 + assert stats_by_month[1].count == 30 From 98b762869d25a6454b374c2e81e354b2e37d2d75 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Thu, 9 Nov 2017 10:41:43 +0000 Subject: [PATCH 17/35] Renamed database migration file after merge Updated the database migration file name after a rebase as there were two 0134 files and this would break the upgrade / downgrade process. --- ...ats_template_usage.py => 0135_stats_template_usage.py} | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) rename migrations/versions/{0134_stats_template_usage.py => 0135_stats_template_usage.py} (94%) diff --git a/migrations/versions/0134_stats_template_usage.py b/migrations/versions/0135_stats_template_usage.py similarity index 94% rename from migrations/versions/0134_stats_template_usage.py rename to migrations/versions/0135_stats_template_usage.py index 4a1fd1963..722fc69ce 100644 --- a/migrations/versions/0134_stats_template_usage.py +++ b/migrations/versions/0135_stats_template_usage.py @@ -1,7 +1,7 @@ """ -Revision ID: 0134_stats_template_usage -Revises: 0133_set_services_sms_prefix +Revision ID: 0135_stats_template_usage +Revises: 0134_add_email_2fa_template Create Date: 2017-11-07 14:35:04.798561 """ @@ -9,8 +9,8 @@ from alembic import op import sqlalchemy as sa from sqlalchemy.dialects import postgresql -revision = '0134_stats_template_usage' -down_revision = '0133_set_services_sms_prefix' +revision = '0135_stats_template_usage' +down_revision = '0134_add_email_2fa_template' def upgrade(): From 23d8d54e0b135c5b2dd35aa63ff1ee69a0bb88e1 Mon Sep 17 00:00:00 2001 From: venusbb Date: Thu, 9 Nov 2017 11:07:11 +0000 Subject: [PATCH 18/35] remove check to financial_year_start in schema as it can be None --- app/billing/billing_schemas.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/billing/billing_schemas.py b/app/billing/billing_schemas.py index 631c782f7..d5a840184 100644 --- a/app/billing/billing_schemas.py +++ b/app/billing/billing_schemas.py @@ -8,7 +8,6 @@ create_or_update_free_sms_fragment_limit_schema = { "title": "Create", "properties": { "free_sms_fragment_limit": {"type": "integer", "minimum": 1}, - "financial_year_start": {"type": "integer", "minimum": 2016} }, "required": ["free_sms_fragment_limit"] } From 59df6bdbb6e72c23115c0ad507733d9fd34d1a48 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Thu, 9 Nov 2017 11:33:56 +0000 Subject: [PATCH 19/35] Fixed imports which were producing broken tests - Some tests for failing because of an import which was overriding the one above it in test_scheduled_tasks.py - Import error fixed in test_services_dao.py after a rename of a method --- tests/app/celery/test_csv_files/email.csv | 2 ++ tests/app/celery/test_csv_files/empty.csv | 1 + tests/app/celery/test_csv_files/multiple_email.csv | 11 +++++++++++ tests/app/celery/test_csv_files/multiple_letter.csv | 11 +++++++++++ tests/app/celery/test_csv_files/multiple_sms.csv | 11 +++++++++++ tests/app/celery/test_csv_files/sms.csv | 2 ++ tests/app/celery/test_scheduled_tasks.py | 11 ++++------- tests/app/dao/test_services_dao.py | 5 +++-- 8 files changed, 45 insertions(+), 9 deletions(-) create mode 100644 tests/app/celery/test_csv_files/email.csv create mode 100644 tests/app/celery/test_csv_files/empty.csv create mode 100644 tests/app/celery/test_csv_files/multiple_email.csv create mode 100644 tests/app/celery/test_csv_files/multiple_letter.csv create mode 100644 tests/app/celery/test_csv_files/multiple_sms.csv create mode 100644 tests/app/celery/test_csv_files/sms.csv diff --git a/tests/app/celery/test_csv_files/email.csv b/tests/app/celery/test_csv_files/email.csv new file mode 100644 index 000000000..f0cefee69 --- /dev/null +++ b/tests/app/celery/test_csv_files/email.csv @@ -0,0 +1,2 @@ +email_address +test@test.com diff --git a/tests/app/celery/test_csv_files/empty.csv b/tests/app/celery/test_csv_files/empty.csv new file mode 100644 index 000000000..64e5bd0a3 --- /dev/null +++ b/tests/app/celery/test_csv_files/empty.csv @@ -0,0 +1 @@ +phone number diff --git a/tests/app/celery/test_csv_files/multiple_email.csv b/tests/app/celery/test_csv_files/multiple_email.csv new file mode 100644 index 000000000..5da15797d --- /dev/null +++ b/tests/app/celery/test_csv_files/multiple_email.csv @@ -0,0 +1,11 @@ +EMAILADDRESS +test1@test.com +test2@test.com +test3@test.com +test4@test.com +test5@test.com +test6@test.com +test7@test.com +test8@test.com +test9@test.com +test0@test.com diff --git a/tests/app/celery/test_csv_files/multiple_letter.csv b/tests/app/celery/test_csv_files/multiple_letter.csv new file mode 100644 index 000000000..0e9d847b8 --- /dev/null +++ b/tests/app/celery/test_csv_files/multiple_letter.csv @@ -0,0 +1,11 @@ +address_line_1, address_line_2, address_line_3 +name1, street1, town1, postcode1 +name2, street2, town2, postcode2 +name3, street3, town3, postcode3 +name4, street4, town4, postcode4 +name5, street5, town5, postcode5 +name6, street6, town6, postcode6 +name7, street7, town7, postcode7 +name8, street8, town8, postcode8 +name9, street9, town9, postcode9 +name0, street0, town0, postcode0 \ No newline at end of file diff --git a/tests/app/celery/test_csv_files/multiple_sms.csv b/tests/app/celery/test_csv_files/multiple_sms.csv new file mode 100644 index 000000000..2ecad9140 --- /dev/null +++ b/tests/app/celery/test_csv_files/multiple_sms.csv @@ -0,0 +1,11 @@ +PhoneNumber,Name ++441234123121,chris ++441234123122,chris ++441234123123,chris ++441234123124,chris ++441234123125,chris ++441234123126,chris ++441234123127,chris ++441234123128,chris ++441234123129,chris ++441234123120,chris diff --git a/tests/app/celery/test_csv_files/sms.csv b/tests/app/celery/test_csv_files/sms.csv new file mode 100644 index 000000000..728639972 --- /dev/null +++ b/tests/app/celery/test_csv_files/sms.csv @@ -0,0 +1,2 @@ +PHONE NUMBER, IGNORE THIS COLUMN ++441234123123, nope diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 7f60318cc..6f63f7396 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -56,19 +56,16 @@ from app.models import ( from app.utils import get_london_midnight_in_utc from app.v2.errors import JobIncompleteError from tests.app.db import create_notification, create_service, create_template, create_job, create_rate + from tests.app.conftest import ( sample_job as create_sample_job, + sample_notification_history as create_notification_history, + sample_template as create_sample_template, create_custom_template, datetime_in_past ) from tests.app.aws.test_s3 import single_s3_object_stub -from tests.conftest import set_config_values, notify_db, notify_db_session - -from tests.app.conftest import ( - sample_notification as create_notification, - sample_notification_history as create_notification_history, - sample_template as create_sample_template -) +from tests.conftest import set_config_values def _create_slow_delivery_notification(provider='mmg'): diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index bc5a8af82..8a28e13dd 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -31,7 +31,8 @@ from app.dao.services_dao import ( dao_resume_service, dao_fetch_active_users_for_service, dao_fetch_service_by_inbound_number, - dao_fetch_monthly_historical_stats_by_template_for_service_without_status) + dao_fetch_monthly_historical_stats_by_template +) from app.dao.service_permissions_dao import dao_add_service_permission, dao_remove_service_permission from app.dao.users_dao import save_model_user from app.models import ( @@ -1022,7 +1023,7 @@ def test_dao_fetch_monthly_historical_stats_by_template(notify_db, notify_db_ses notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) notification_history(created_at=datetime.now(), sample_template=template_two) - results = dao_fetch_monthly_historical_stats_by_template_for_service_without_status() + results = dao_fetch_monthly_historical_stats_by_template() assert len(results) == 2 From b78d989d4eb144760d7e1deb8fb65f1a2111c1a4 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Thu, 9 Nov 2017 14:13:42 +0000 Subject: [PATCH 20/35] Updates after review - Modified the services_dao to return an int instead of a datetime to make usage easier and removed the BST function on year as it is not relevant for year - Improved tests do there is less logic by ordering the result so there is less reliance on the template id - Renamed variable in stats_template_usage_by_month_dao.py to make it consistent with the method --- app/celery/scheduled_tasks.py | 6 +- app/config.py | 2 +- app/dao/services_dao.py | 8 +- app/dao/stats_template_usage_by_month_dao.py | 4 +- app/utils.py | 18 +-- .../versions/0135_stats_template_usage.py | 14 --- tests/app/celery/test_csv_files/email.csv | 2 - tests/app/celery/test_csv_files/empty.csv | 1 - .../celery/test_csv_files/multiple_email.csv | 11 -- .../celery/test_csv_files/multiple_letter.csv | 11 -- .../celery/test_csv_files/multiple_sms.csv | 11 -- tests/app/celery/test_csv_files/sms.csv | 2 - tests/app/celery/test_scheduled_tasks.py | 104 +++++++++--------- tests/app/dao/test_services_dao.py | 30 +++-- 14 files changed, 76 insertions(+), 148 deletions(-) delete mode 100644 tests/app/celery/test_csv_files/email.csv delete mode 100644 tests/app/celery/test_csv_files/empty.csv delete mode 100644 tests/app/celery/test_csv_files/multiple_email.csv delete mode 100644 tests/app/celery/test_csv_files/multiple_letter.csv delete mode 100644 tests/app/celery/test_csv_files/multiple_sms.csv delete mode 100644 tests/app/celery/test_csv_files/sms.csv diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 9b1a830d9..e39876df0 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -410,13 +410,13 @@ def check_job_status(): @notify_celery.task(name='daily-stats-template_usage_by_month') @statsd(namespace="tasks") -def daily_stats_template_usage_my_month(): +def daily_stats_template_usage_by_month(): results = dao_fetch_monthly_historical_stats_by_template() for result in results: insert_or_update_stats_for_template( result.template_id, - result.month.month, - result.year.year, + result.month, + result.year, result.count ) diff --git a/app/config.py b/app/config.py index afd70dade..16892ac28 100644 --- a/app/config.py +++ b/app/config.py @@ -244,7 +244,7 @@ class Config(object): }, 'daily-stats-template_usage_by_month': { 'task': 'daily-stats-template_usage_by_month', - 'schedule': crontab(hour=00, minute=50), + 'schedule': crontab(hour=0, minute=50), 'options': {'queue': QueueNames.PERIODIC} } } diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 684696d83..d023fb297 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -40,7 +40,7 @@ from app.models import ( ) from app.service.statistics import format_monthly_template_notification_stats from app.statsd_decorators import statsd -from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc, get_london_year_from_utc_column +from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc from app.dao.annual_billing_dao import dao_insert_annual_billing DEFAULT_SERVICE_PERMISSIONS = [ @@ -525,13 +525,13 @@ def dao_fetch_active_users_for_service(service_id): @statsd(namespace="dao") def dao_fetch_monthly_historical_stats_by_template(): month = get_london_month_from_utc_column(NotificationHistory.created_at) - year = get_london_year_from_utc_column(NotificationHistory.created_at) + year = func.date_trunc("year", NotificationHistory.created_at) end_date = datetime.combine(date.today(), time.min) return db.session.query( NotificationHistory.template_id, - month.label('month'), - year.label('year'), + extract('month', month).label('month'), + extract('year', year).label('year'), func.count().label('count') ).filter( NotificationHistory.created_at < end_date diff --git a/app/dao/stats_template_usage_by_month_dao.py b/app/dao/stats_template_usage_by_month_dao.py index ca94dd83f..2884e2c53 100644 --- a/app/dao/stats_template_usage_by_month_dao.py +++ b/app/dao/stats_template_usage_by_month_dao.py @@ -15,10 +15,10 @@ def insert_or_update_stats_for_template(template_id, month, year, count): } ) if result == 0: - new_sms_sender = StatsTemplateUsageByMonth( + monthly_stats = StatsTemplateUsageByMonth( template_id=template_id, month=month, year=year, count=count ) - db.session.add(new_sms_sender) + db.session.add(monthly_stats) diff --git a/app/utils.py b/app/utils.py index 7fe4bdf89..ffb628284 100644 --- a/app/utils.py +++ b/app/utils.py @@ -71,23 +71,7 @@ def get_london_month_from_utc_column(column): """ return func.date_trunc( "month", - func.timezone("Europe/London", func.timezone("UTC", column)) - ) - - -def get_london_year_from_utc_column(column): - """ - Where queries need to count notifications by month it needs to be - the month in BST (British Summer Time). - The database stores all timestamps as UTC without the timezone. - - First set the timezone on created_at to UTC - - then convert the timezone to BST (or Europe/London) - - lastly truncate the datetime to month with which we can group - queries - """ - return func.date_trunc( - "month", - func.timezone("Europe/London", func.timezone("UTC", column)) + func.timezone("Europe/London", column) ) diff --git a/migrations/versions/0135_stats_template_usage.py b/migrations/versions/0135_stats_template_usage.py index 722fc69ce..5a8f5ef7a 100644 --- a/migrations/versions/0135_stats_template_usage.py +++ b/migrations/versions/0135_stats_template_usage.py @@ -31,20 +31,6 @@ def upgrade(): def downgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.create_table('notification_statistics', - sa.Column('id', postgresql.UUID(), autoincrement=False, nullable=False), - sa.Column('service_id', postgresql.UUID(), autoincrement=False, nullable=False), - sa.Column('emails_requested', sa.BIGINT(), autoincrement=False, nullable=False), - sa.Column('emails_delivered', sa.BIGINT(), autoincrement=False, nullable=False), - sa.Column('emails_failed', sa.BIGINT(), autoincrement=False, nullable=False), - sa.Column('sms_requested', sa.BIGINT(), autoincrement=False, nullable=False), - sa.Column('sms_delivered', sa.BIGINT(), autoincrement=False, nullable=False), - sa.Column('sms_failed', sa.BIGINT(), autoincrement=False, nullable=False), - sa.Column('day', sa.DATE(), autoincrement=False, nullable=False), - sa.ForeignKeyConstraint(['service_id'], ['services.id'], name='notification_statistics_service_id_fkey'), - sa.PrimaryKeyConstraint('id', name='notification_statistics_pkey'), - sa.UniqueConstraint('service_id', 'day', name='uix_service_to_day') - ) op.drop_index(op.f('ix_stats_template_usage_by_month_year'), table_name='stats_template_usage_by_month') op.drop_index(op.f('ix_stats_template_usage_by_month_template_id'), table_name='stats_template_usage_by_month') op.drop_index(op.f('ix_stats_template_usage_by_month_month'), table_name='stats_template_usage_by_month') diff --git a/tests/app/celery/test_csv_files/email.csv b/tests/app/celery/test_csv_files/email.csv deleted file mode 100644 index f0cefee69..000000000 --- a/tests/app/celery/test_csv_files/email.csv +++ /dev/null @@ -1,2 +0,0 @@ -email_address -test@test.com diff --git a/tests/app/celery/test_csv_files/empty.csv b/tests/app/celery/test_csv_files/empty.csv deleted file mode 100644 index 64e5bd0a3..000000000 --- a/tests/app/celery/test_csv_files/empty.csv +++ /dev/null @@ -1 +0,0 @@ -phone number diff --git a/tests/app/celery/test_csv_files/multiple_email.csv b/tests/app/celery/test_csv_files/multiple_email.csv deleted file mode 100644 index 5da15797d..000000000 --- a/tests/app/celery/test_csv_files/multiple_email.csv +++ /dev/null @@ -1,11 +0,0 @@ -EMAILADDRESS -test1@test.com -test2@test.com -test3@test.com -test4@test.com -test5@test.com -test6@test.com -test7@test.com -test8@test.com -test9@test.com -test0@test.com diff --git a/tests/app/celery/test_csv_files/multiple_letter.csv b/tests/app/celery/test_csv_files/multiple_letter.csv deleted file mode 100644 index 0e9d847b8..000000000 --- a/tests/app/celery/test_csv_files/multiple_letter.csv +++ /dev/null @@ -1,11 +0,0 @@ -address_line_1, address_line_2, address_line_3 -name1, street1, town1, postcode1 -name2, street2, town2, postcode2 -name3, street3, town3, postcode3 -name4, street4, town4, postcode4 -name5, street5, town5, postcode5 -name6, street6, town6, postcode6 -name7, street7, town7, postcode7 -name8, street8, town8, postcode8 -name9, street9, town9, postcode9 -name0, street0, town0, postcode0 \ No newline at end of file diff --git a/tests/app/celery/test_csv_files/multiple_sms.csv b/tests/app/celery/test_csv_files/multiple_sms.csv deleted file mode 100644 index 2ecad9140..000000000 --- a/tests/app/celery/test_csv_files/multiple_sms.csv +++ /dev/null @@ -1,11 +0,0 @@ -PhoneNumber,Name -+441234123121,chris -+441234123122,chris -+441234123123,chris -+441234123124,chris -+441234123125,chris -+441234123126,chris -+441234123127,chris -+441234123128,chris -+441234123129,chris -+441234123120,chris diff --git a/tests/app/celery/test_csv_files/sms.csv b/tests/app/celery/test_csv_files/sms.csv deleted file mode 100644 index 728639972..000000000 --- a/tests/app/celery/test_csv_files/sms.csv +++ /dev/null @@ -1,2 +0,0 @@ -PHONE NUMBER, IGNORE THIS COLUMN -+441234123123, nope diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 6f63f7396..d31e2b5a4 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -33,7 +33,8 @@ from app.celery.scheduled_tasks import ( switch_current_sms_provider_on_slow_delivery, timeout_job_statistics, timeout_notifications, - daily_stats_template_usage_my_month) + daily_stats_template_usage_by_month +) from app.clients.performance_platform.performance_platform_client import PerformancePlatformClient from app.config import QueueNames, TaskNames from app.dao.jobs_dao import dao_get_job_by_id @@ -840,7 +841,7 @@ def test_check_job_status_task_raises_job_incomplete_error_for_multiple_jobs(moc ) -def test_daily_stats_template_usage_my_month(notify_db, notify_db_session): +def test_daily_stats_template_usage_by_month(notify_db, notify_db_session): notification_history = functools.partial( create_notification_history, notify_db, @@ -856,36 +857,37 @@ def test_daily_stats_template_usage_my_month(notify_db, notify_db_session): notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) notification_history(created_at=datetime.now(), sample_template=template_two) - daily_stats_template_usage_my_month() + daily_stats_template_usage_by_month() - results = db.session.query(StatsTemplateUsageByMonth).all() + result = db.session.query( + StatsTemplateUsageByMonth + ).order_by( + StatsTemplateUsageByMonth.year, + StatsTemplateUsageByMonth.month + ).all() - assert len(results) == 2 + assert len(result) == 2 - for result in results: - if result.template_id == template_one.id: - assert result.template_id == template_one.id - assert result.month == 10 - assert result.year == 2017 - assert result.count == 1 - elif result.template_id == template_two.id: - assert result.template_id == template_two.id - assert result.month == 4 - assert result.year == 2016 - assert result.count == 2 - else: - raise AssertionError() + assert result[0].template_id == template_two.id + assert result[0].month == 4 + assert result[0].year == 2016 + assert result[0].count == 2 + + assert result[1].template_id == template_one.id + assert result[1].month == 10 + assert result[1].year == 2017 + assert result[1].count == 1 -def test_daily_stats_template_usage_my_month_no_data(): - daily_stats_template_usage_my_month() +def test_daily_stats_template_usage_by_month_no_data(): + daily_stats_template_usage_by_month() results = db.session.query(StatsTemplateUsageByMonth).all() assert len(results) == 0 -def test_daily_stats_template_usage_my_month_multiple_runs(notify_db, notify_db_session): +def test_daily_stats_template_usage_by_month_multiple_runs(notify_db, notify_db_session): notification_history = functools.partial( create_notification_history, notify_db, @@ -896,12 +898,12 @@ def test_daily_stats_template_usage_my_month_multiple_runs(notify_db, notify_db_ template_one = create_sample_template(notify_db, notify_db_session) template_two = create_sample_template(notify_db, notify_db_session) - notification_history(created_at=datetime(2017, 10, 1), sample_template=template_one) + notification_history(created_at=datetime(2017, 11, 1), sample_template=template_one) notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) notification_history(created_at=datetime.now(), sample_template=template_two) - daily_stats_template_usage_my_month() + daily_stats_template_usage_by_month() template_three = create_sample_template(notify_db, notify_db_session) @@ -911,35 +913,33 @@ def test_daily_stats_template_usage_my_month_multiple_runs(notify_db, notify_db_ notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) notification_history(created_at=datetime.now(), sample_template=template_two) - daily_stats_template_usage_my_month() + daily_stats_template_usage_by_month() - results = db.session.query(StatsTemplateUsageByMonth).all() + result = db.session.query( + StatsTemplateUsageByMonth + ).order_by( + StatsTemplateUsageByMonth.year, + StatsTemplateUsageByMonth.month + ).all() - assert len(results) == 4 + assert len(result) == 4 - for result in results: - if result.template_id == template_one.id: - assert result.template_id == template_one.id - assert result.month == 10 - assert result.year == 2017 - assert result.count == 1 - elif result.template_id == template_two.id: - assert result.template_id == template_two.id - assert result.month == 4 - assert result.year == 2016 - assert result.count == 4 - elif result.template_id == template_three.id: - if result.month == 10: - assert result.template_id == template_three.id - assert result.month == 10 - assert result.year == 2017 - assert result.count == 1 - elif result.month == 9: - assert result.template_id == template_three.id - assert result.month == 9 - assert result.year == 2017 - assert result.count == 1 - else: - raise AssertionError() - else: - raise AssertionError() + assert result[0].template_id == template_two.id + assert result[0].month == 4 + assert result[0].year == 2016 + assert result[0].count == 4 + + assert result[1].template_id == template_three.id + assert result[1].month == 9 + assert result[1].year == 2017 + assert result[1].count == 1 + + assert result[2].template_id == template_three.id + assert result[2].month == 10 + assert result[2].year == 2017 + assert result[2].count == 1 + + assert result[3].template_id == template_one.id + assert result[3].month == 11 + assert result[3].year == 2017 + assert result[3].count == 1 diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 8a28e13dd..997301531 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -1015,28 +1015,24 @@ def test_dao_fetch_monthly_historical_stats_by_template(notify_db, notify_db_ses status='delivered' ) - template_one = create_sample_template(notify_db, notify_db_session) - template_two = create_sample_template(notify_db, notify_db_session) + template_one = create_sample_template(notify_db, notify_db_session, template_name='1') + template_two = create_sample_template(notify_db, notify_db_session, template_name='2') notification_history(created_at=datetime(2017, 10, 1), sample_template=template_one) notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) notification_history(created_at=datetime.now(), sample_template=template_two) - results = dao_fetch_monthly_historical_stats_by_template() + result = sorted(dao_fetch_monthly_historical_stats_by_template(), key=lambda x: (x.month, x.year)) - assert len(results) == 2 + assert len(result) == 2 - for result in results: - if result.template_id == template_one.id: - assert result.template_id == template_one.id - assert result.month.month == 10 - assert result.year.year == 2017 - assert result.count == 1 - elif result.template_id == template_two.id: - assert result.template_id == template_two.id - assert result.month.month == 4 - assert result.year.year == 2016 - assert result.count == 2 - else: - raise AssertionError() + assert result[0].template_id == template_two.id + assert result[0].month == 4 + assert result[0].year == 2016 + assert result[0].count == 2 + + assert result[1].template_id == template_one.id + assert result[1].month == 10 + assert result[1].year == 2017 + assert result[1].count == 1 From 048ddbb2a417b092ce2535b196442feea8304ef0 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Nov 2017 14:27:24 +0000 Subject: [PATCH 21/35] add separate activate user endpoint --- app/user/rest.py | 11 +++++++++++ tests/app/user/test_rest.py | 16 ++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/app/user/rest.py b/app/user/rest.py index b6c2c28e0..5b559e606 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -91,6 +91,17 @@ def update_user_attribute(user_id): return jsonify(data=user_schema.dump(user_to_update).data), 200 +@user_blueprint.route('//activate', methods=['POST']) +def activate_user(user_id): + user = get_user_by_id(user_id=user_id) + if user.state == 'active': + raise InvalidRequest('User already active', status_code=400) + + user.state = 'active' + save_model_user(user) + return jsonify(data=user_schema.dump(user).data), 200 + + @user_blueprint.route('//reset-failed-login-count', methods=['POST']) def user_reset_failed_login_count(user_id): user_to_update = get_user_by_id(user_id=user_id) diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 2a36d488e..25fd5fde0 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -558,3 +558,19 @@ def test_update_user_auth_type(admin_request, sample_user): assert resp['data']['id'] == str(sample_user.id) assert resp['data']['auth_type'] == 'email_auth' + + +def test_activate_user(admin_request, sample_user): + sample_user.state = 'pending' + + resp = admin_request.post('user.activate_user', user_id=sample_user.id) + + assert resp['data']['id'] == str(sample_user.id) + assert resp['data']['state'] == 'active' + assert sample_user.state == 'active' + + +def test_activate_user_fails_if_already_active(admin_request, sample_user): + resp = admin_request.post('user.activate_user', user_id=sample_user.id, _expected_status=400) + assert resp['message'] == 'User already active' + assert sample_user.state == 'active' From 15e86170d9eb94c2fdbeb7ea617cdabfc50a0d94 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Thu, 9 Nov 2017 14:55:45 +0000 Subject: [PATCH 22/35] Changed timezone back Reverted back the timezoe change that was made in error and was making two tests fail. --- app/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/utils.py b/app/utils.py index ffb628284..49f6d61dc 100644 --- a/app/utils.py +++ b/app/utils.py @@ -71,7 +71,7 @@ def get_london_month_from_utc_column(column): """ return func.date_trunc( "month", - func.timezone("Europe/London", column) + func.timezone("Europe/London", func.timezone("UTC", column)) ) From cd4637a1d7e378d06d9e10060e3e9b9c40764ae4 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 9 Nov 2017 15:22:27 +0000 Subject: [PATCH 23/35] Removed provider_reference and provider_date from serialize --- app/models.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models.py b/app/models.py index ea7b93a4e..81cc4d727 100644 --- a/app/models.py +++ b/app/models.py @@ -1410,8 +1410,6 @@ class InboundSms(db.Model): 'notify_number': self.notify_number, 'user_number': self.user_number, 'content': self.content, - 'provider_date': self.provider_date and self.provider_date.strftime(DATETIME_FORMAT), - 'provider_reference': self.provider_reference } From 61a9f6e9507757cc29496479ab5480f0ee86e7df Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 9 Nov 2017 15:22:53 +0000 Subject: [PATCH 24/35] Removed provider_date and provider_reference from schema --- app/v2/inbound_sms/inbound_sms_schemas.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/app/v2/inbound_sms/inbound_sms_schemas.py b/app/v2/inbound_sms/inbound_sms_schemas.py index e6ef8cc98..e68952a23 100644 --- a/app/v2/inbound_sms/inbound_sms_schemas.py +++ b/app/v2/inbound_sms/inbound_sms_schemas.py @@ -18,12 +18,6 @@ get_inbound_sms_single_response = { "type": "object", "title": "GET response v2/inbound_sms", "properties": { - "provider_date": { - "format": "date-time", - "type": "string", - "description": "Date+time sent by provider" - }, - "provider_reference": {"type": ["string", "null"]}, "user_number": {"type": "string"}, "created_at": { "format": "date-time", @@ -36,8 +30,7 @@ get_inbound_sms_single_response = { "content": {"type": "string"}, }, "required": [ - "id", "provider_date", "provider_reference", - "user_number", "created_at", "service_id", + "id", "user_number", "created_at", "service_id", "notify_number", "content" ], "additionalProperties": False, From 36fb2fb28cbe78ff0ad9008cf1ffb1e017376852 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 9 Nov 2017 15:23:16 +0000 Subject: [PATCH 25/35] Remove user_number from inbound sms dao --- app/dao/inbound_sms_dao.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 4813fc695..f7d54281e 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -34,7 +34,6 @@ def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None): def dao_get_paginated_inbound_sms_for_service( service_id, - user_number=None, older_than=None, page_size=None ): @@ -48,9 +47,6 @@ def dao_get_paginated_inbound_sms_for_service( InboundSms.created_at).filter(InboundSms.id == older_than).as_scalar() filters.append(InboundSms.created_at < older_than_created_at) - if user_number: - filters.append(InboundSms.user_number == user_number) - query = InboundSms.query.filter(*filters) return query.order_by(desc(InboundSms.created_at)).paginate( From 362072129fa83df38e7b3cc9e57a78d2032c6d16 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 9 Nov 2017 15:23:45 +0000 Subject: [PATCH 26/35] Add tests for dao_get_paginated_inbound_sms_for_service --- tests/app/dao/test_inbound_sms_dao.py | 85 +++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 5 deletions(-) diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index b26dc913e..7967d8522 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -6,7 +6,8 @@ from app.dao.inbound_sms_dao import ( dao_get_inbound_sms_for_service, dao_count_inbound_sms_for_service, delete_inbound_sms_created_more_than_a_week_ago, - dao_get_inbound_sms_by_id + dao_get_inbound_sms_by_id, + dao_get_paginated_inbound_sms_for_service ) from tests.app.db import create_inbound_sms, create_service @@ -89,9 +90,83 @@ def test_should_not_delete_inbound_sms_before_seven_days(sample_service): assert len(InboundSms.query.all()) == 2 -def test_get_inbound_sms_by_id_returns(sample_service): - inbound = create_inbound_sms(sample_service) +def test_get_inbound_sms_by_id_returns(sample_inbound_sms): + inbound_from_db = dao_get_inbound_sms_by_id(sample_inbound_sms.service.id, sample_inbound_sms.id) - inbound_from_db = dao_get_inbound_sms_by_id(sample_service.id, inbound.id) + assert sample_inbound_sms == inbound_from_db - assert inbound == inbound_from_db + +def test_dao_get_paginated_inbound_sms_for_service(sample_inbound_sms): + inbound_from_db = dao_get_paginated_inbound_sms_for_service(sample_inbound_sms.service.id) + + assert sample_inbound_sms == inbound_from_db[0] + + +def test_dao_get_paginated_inbound_sms_for_service_return_only_for_service(sample_inbound_sms): + another_service = create_service(service_name='another service') + another_inbound_sms = create_inbound_sms(another_service) + + inbound_from_db = dao_get_paginated_inbound_sms_for_service(sample_inbound_sms.service.id) + + assert sample_inbound_sms in inbound_from_db + assert another_inbound_sms not in inbound_from_db + + +def test_dao_get_paginated_inbound_sms_for_service_no_inbound_sms_returns_empty_list(sample_service): + inbound_from_db = dao_get_paginated_inbound_sms_for_service(sample_service.id) + + assert inbound_from_db == [] + + +def test_dao_get_paginated_inbound_sms_for_service_page_size_returns_correct_size(sample_service): + inbound_sms_list = [ + create_inbound_sms(sample_service), + create_inbound_sms(sample_service), + create_inbound_sms(sample_service), + create_inbound_sms(sample_service), + ] + reversed_inbound_sms = sorted(inbound_sms_list, key=lambda sms: sms.created_at, reverse=True) + + inbound_from_db = dao_get_paginated_inbound_sms_for_service( + sample_service.id, + older_than=reversed_inbound_sms[1].id, + page_size=2 + ) + + assert len(inbound_from_db) == 2 + + +def test_dao_get_paginated_inbound_sms_for_service_older_than_returns_correct_list(sample_service): + inbound_sms_list = [ + create_inbound_sms(sample_service), + create_inbound_sms(sample_service), + create_inbound_sms(sample_service), + create_inbound_sms(sample_service), + ] + reversed_inbound_sms = sorted(inbound_sms_list, key=lambda sms: sms.created_at, reverse=True) + + inbound_from_db = dao_get_paginated_inbound_sms_for_service( + sample_service.id, + older_than=reversed_inbound_sms[1].id, + page_size=2 + ) + + expected_inbound_sms = reversed_inbound_sms[2:] + + assert expected_inbound_sms == inbound_from_db + + +def test_dao_get_paginated_inbound_sms_for_service_older_than_end_returns_empty_list(sample_service): + inbound_sms_list = [ + create_inbound_sms(sample_service), + create_inbound_sms(sample_service), + ] + reversed_inbound_sms = sorted(inbound_sms_list, key=lambda sms: sms.created_at, reverse=True) + + inbound_from_db = dao_get_paginated_inbound_sms_for_service( + sample_service.id, + older_than=reversed_inbound_sms[1].id, + page_size=2 + ) + + assert inbound_from_db == [] From aac7551fc82336fc2e02f0a2af079c143dc9db4a Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 9 Nov 2017 15:24:08 +0000 Subject: [PATCH 27/35] Removed provider_date and provider_reference --- tests/app/inbound_sms/test_rest.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/app/inbound_sms/test_rest.py b/tests/app/inbound_sms/test_rest.py index 9d2c891f2..a693307ef 100644 --- a/tests/app/inbound_sms/test_rest.py +++ b/tests/app/inbound_sms/test_rest.py @@ -33,9 +33,7 @@ def test_get_inbound_sms_with_no_params(client, sample_service): 'service_id', 'notify_number', 'user_number', - 'content', - 'provider_date', - 'provider_reference' + 'content' } @@ -178,9 +176,7 @@ def test_get_inbound_sms(admin_request, sample_service): 'service_id', 'notify_number', 'user_number', - 'content', - 'provider_date', - 'provider_reference' + 'content' } From 99c2ccb496f8ac77e86013c780af4f1cee33022a Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Thu, 9 Nov 2017 15:24:30 +0000 Subject: [PATCH 28/35] Refactor schema --- tests/app/v2/inbound_sms/test_inbound_sms_schemas.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py index fe9613056..c2e32f93a 100644 --- a/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py +++ b/tests/app/v2/inbound_sms/test_inbound_sms_schemas.py @@ -13,8 +13,6 @@ from tests import create_authorization_header from tests.app.db import create_inbound_sms valid_inbound_sms = { - "provider_date": "2017-11-02T15:07:57.199541Z", - "provider_reference": "foo", "user_number": "447700900111", "created_at": "2017-11-02T15:07:57.197546Z", "service_id": "a5149c32-f03b-4711-af49-ad6993797d45", @@ -32,8 +30,6 @@ valid_inbound_sms_list = { } invalid_inbound_sms = { - "provider_date": "2017-11-02T15:07:57.199541", - "provider_reference": "foo", "user_number": "447700900111", "created_at": "2017-11-02T15:07:57.197546", "service_id": "a5149c32-f03b-4711-af49-ad6993797d45", From 8ef4e1e1da1919e1a8406fa6e0d5173899944c62 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Nov 2017 16:48:05 +0000 Subject: [PATCH 29/35] remove trailing underscore, which is sometimes created by alembic --- scripts/check_if_new_migration.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/check_if_new_migration.py b/scripts/check_if_new_migration.py index ba0519ea1..2cd1614d0 100644 --- a/scripts/check_if_new_migration.py +++ b/scripts/check_if_new_migration.py @@ -8,7 +8,8 @@ def get_latest_db_migration_to_apply(): project_dir = dirname(dirname(abspath(__file__))) # Get the main project directory migrations_dir = '{}/migrations/versions/'.format(project_dir) migration_files = [migration_file for migration_file in os.listdir(migrations_dir) if migration_file.endswith('py')] - latest_file = sorted(migration_files, reverse=True)[0].replace('.py', '') + # sometimes there's a trailing underscore, if script was created with `python app.py db migrate --rev-id=...` + latest_file = sorted(migration_files, reverse=True)[0].replace('_.py', '').replace('.py', '') return latest_file From 15bf888624bc2fe513b3427eb3c3f5fa8dc3c10f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Nov 2017 14:18:47 +0000 Subject: [PATCH 30/35] make user mobile num nullable if user has email_auth enabled --- app/errors.py | 2 +- app/models.py | 9 +- app/schemas.py | 23 +++- app/user/rest.py | 14 ++ .../versions/0136_user_mobile_nullable.py | 28 ++++ tests/app/user/test_rest.py | 126 ++++++++++++++++-- 6 files changed, 185 insertions(+), 17 deletions(-) create mode 100644 migrations/versions/0136_user_mobile_nullable.py diff --git a/app/errors.py b/app/errors.py index a6119d98d..8ff935002 100644 --- a/app/errors.py +++ b/app/errors.py @@ -94,7 +94,7 @@ def register_errors(blueprint): current_app.logger.exception(e) if hasattr(e, 'orig') and hasattr(e.orig, 'pgerror') and e.orig.pgerror and \ ('duplicate key value violates unique constraint "services_name_key"' in e.orig.pgerror or - 'duplicate key value violates unique constraint "services_email_from_key"' in e.orig.pgerror): + 'duplicate key value violates unique constraint "services_email_from_key"' in e.orig.pgerror): return jsonify( result='error', message={'name': ["Duplicate service name '{}'".format( diff --git a/app/models.py b/app/models.py index 0c64a4581..bd6436b05 100644 --- a/app/models.py +++ b/app/models.py @@ -9,7 +9,7 @@ from sqlalchemy.dialects.postgresql import ( UUID, JSON ) -from sqlalchemy import UniqueConstraint, and_ +from sqlalchemy import UniqueConstraint, CheckConstraint, and_ from sqlalchemy.orm import foreign, remote from notifications_utils.recipients import ( validate_email_address, @@ -97,7 +97,7 @@ class User(db.Model): nullable=True, onupdate=datetime.datetime.utcnow) _password = db.Column(db.String, index=False, unique=False, nullable=False) - mobile_number = db.Column(db.String, index=False, unique=False, nullable=False) + mobile_number = db.Column(db.String, index=False, unique=False, nullable=True) password_changed_at = db.Column(db.DateTime, index=False, unique=False, nullable=False, default=datetime.datetime.utcnow) logged_in_at = db.Column(db.DateTime, nullable=True) @@ -107,6 +107,9 @@ class User(db.Model): current_session_id = db.Column(UUID(as_uuid=True), nullable=True) auth_type = db.Column(db.String, db.ForeignKey('auth_type.name'), index=True, nullable=False, default=SMS_AUTH_TYPE) + # either email auth or a mobile number must be provided + CheckConstraint("auth_type = 'email_auth' or mobile_number is not null") + services = db.relationship( 'Service', secondary='user_to_service', @@ -560,7 +563,7 @@ class Template(db.Model): nullable=False, default=NORMAL ) - + redact_personalisation = association_proxy('template_redacted', 'redact_personalisation') def get_link(self): diff --git a/app/schemas.py b/app/schemas.py index e689bf83f..fe9d29b0f 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -105,6 +105,26 @@ class UserSchema(BaseSchema): "_password", "verify_codes") strict = True + @validates('name') + def validate_name(self, value): + if not value: + raise ValidationError('Invalid name') + + @validates('email_address') + def validate_email_address(self, value): + try: + validate_email_address(value) + except InvalidEmailError as e: + raise ValidationError(str(e)) + + @validates('mobile_number') + def validate_mobile_number(self, value): + try: + if value is not None: + validate_phone_number(value, international=True) + except InvalidPhoneError as error: + raise ValidationError('Invalid phone number: {}'.format(error)) + class UserUpdateAttributeSchema(BaseSchema): auth_type = field_for(models.User, 'auth_type') @@ -132,7 +152,8 @@ class UserUpdateAttributeSchema(BaseSchema): @validates('mobile_number') def validate_mobile_number(self, value): try: - validate_phone_number(value, international=True) + if value is not None: + validate_phone_number(value, international=True) except InvalidPhoneError as error: raise ValidationError('Invalid phone number: {}'.format(error)) diff --git a/app/user/rest.py b/app/user/rest.py index 5b559e606..acf7e8abb 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -4,6 +4,7 @@ from datetime import datetime from urllib.parse import urlencode from flask import (jsonify, request, Blueprint, current_app, abort) +from sqlalchemy.exc import IntegrityError from app.config import QueueNames from app.dao.users_dao import ( @@ -52,6 +53,19 @@ user_blueprint = Blueprint('user', __name__) register_errors(user_blueprint) +@user_blueprint.errorhandler(IntegrityError) +def handle_integrity_error(exc): + """ + Handle integrity errors caused by the auth type/mobile number check constraint + """ + if 'ck_users_mobile_or_email_auth' in str(exc): + # we don't expect this to trip, so still log error + current_app.logger.exception('Check constraint ck_users_mobile_or_email_auth triggered') + return jsonify(result='error', message='Mobile number must be set if auth_type is set to sms_auth'), 400 + + raise + + @user_blueprint.route('', methods=['POST']) def create_user(): user_to_create, errors = user_schema.load(request.get_json()) diff --git a/migrations/versions/0136_user_mobile_nullable.py b/migrations/versions/0136_user_mobile_nullable.py new file mode 100644 index 000000000..8ce4df31a --- /dev/null +++ b/migrations/versions/0136_user_mobile_nullable.py @@ -0,0 +1,28 @@ +""" + +Revision ID: 0136_user_mobile_nullable +Revises: 0135_stats_template_usage +Create Date: 2017-11-08 11:49:05.773974 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.sql import column +from sqlalchemy.dialects import postgresql + +revision = '0136_user_mobile_nullable' +down_revision = '0135_stats_template_usage' + + +def upgrade(): + op.alter_column('users', 'mobile_number', nullable=True) + + op.create_check_constraint( + 'ck_users_mobile_or_email_auth', + 'users', + "auth_type = 'email_auth' or mobile_number is not null" + ) + +def downgrade(): + op.alter_column('users', 'mobile_number', nullable=False) + op.drop_constraint('ck_users_mobile_or_email_auth', 'users') diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 25fd5fde0..880cf2cd1 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -159,6 +159,55 @@ def test_create_user_missing_attribute_password(client, notify_db, notify_db_ses assert {'password': ['Missing data for required field.']} == json_resp['message'] +def test_can_create_user_with_email_auth_and_no_mobile(admin_request, notify_db_session): + data = { + 'name': 'Test User', + 'email_address': 'user@digital.cabinet-office.gov.uk', + 'password': 'password', + 'mobile_number': None, + 'auth_type': EMAIL_AUTH_TYPE + } + + json_resp = admin_request.post('user.create_user', _data=data, _expected_status=201) + + assert json_resp['data']['auth_type'] == EMAIL_AUTH_TYPE + assert json_resp['data']['mobile_number'] is None + + +def test_cannot_create_user_with_sms_auth_and_no_mobile(admin_request, notify_db_session): + data = { + 'name': 'Test User', + 'email_address': 'user@digital.cabinet-office.gov.uk', + 'password': 'password', + 'mobile_number': None, + 'auth_type': SMS_AUTH_TYPE + } + + json_resp = admin_request.post('user.create_user', _data=data, _expected_status=400) + + assert json_resp['message'] == 'Mobile number must be set if auth_type is set to sms_auth' + + +def test_cannot_create_user_with_empty_strings(admin_request, notify_db_session): + data = { + 'name': '', + 'email_address': '', + 'password': 'password', + 'mobile_number': '', + 'auth_type': EMAIL_AUTH_TYPE + } + resp = admin_request.post( + 'user.create_user', + _data=data, + _expected_status=400 + ) + assert resp['message'] == { + 'email_address': ['Not a valid email address'], + 'mobile_number': ['Invalid phone number: Not enough digits'], + 'name': ['Invalid name'] + } + + def test_put_user(client, sample_service): """ Tests PUT endpoint '/' to update a user. @@ -548,18 +597,6 @@ def test_update_user_resets_failed_login_count_if_updating_password(client, samp assert user.failed_login_count == 0 -def test_update_user_auth_type(admin_request, sample_user): - assert sample_user.auth_type == 'sms_auth' - resp = admin_request.post( - 'user.update_user_attribute', - user_id=sample_user.id, - _data={'auth_type': 'email_auth'}, - ) - - assert resp['data']['id'] == str(sample_user.id) - assert resp['data']['auth_type'] == 'email_auth' - - def test_activate_user(admin_request, sample_user): sample_user.state = 'pending' @@ -574,3 +611,68 @@ def test_activate_user_fails_if_already_active(admin_request, sample_user): resp = admin_request.post('user.activate_user', user_id=sample_user.id, _expected_status=400) assert resp['message'] == 'User already active' assert sample_user.state == 'active' + + +def test_update_user_auth_type(admin_request, sample_user): + assert sample_user.auth_type == 'sms_auth' + resp = admin_request.post( + 'user.update_user_attribute', + user_id=sample_user.id, + _data={'auth_type': 'email_auth'}, + ) + + assert resp['data']['id'] == str(sample_user.id) + assert resp['data']['auth_type'] == 'email_auth' + + +def test_can_set_email_auth_and_remove_mobile_at_same_time(admin_request, sample_user): + sample_user.auth_type = SMS_AUTH_TYPE + + admin_request.post( + 'user.update_user_attribute', + user_id=sample_user.id, + _data={ + 'mobile_number': None, + 'auth_type': EMAIL_AUTH_TYPE, + } + ) + + assert sample_user.mobile_number is None + assert sample_user.auth_type == EMAIL_AUTH_TYPE + + +def test_cannot_remove_mobile_if_sms_auth(admin_request, sample_user): + sample_user.auth_type = SMS_AUTH_TYPE + + json_resp = admin_request.post( + 'user.update_user_attribute', + user_id=sample_user.id, + _data={'mobile_number': None}, + _expected_status=400 + ) + + assert json_resp['message'] == 'Mobile number must be set if auth_type is set to sms_auth' + + +def test_can_remove_mobile_if_email_auth(admin_request, sample_user): + sample_user.auth_type = EMAIL_AUTH_TYPE + + admin_request.post( + 'user.update_user_attribute', + user_id=sample_user.id, + _data={'mobile_number': None}, + ) + + assert sample_user.mobile_number is None + + +def test_cannot_update_user_with_mobile_number_as_empty_string(admin_request, sample_user): + sample_user.auth_type = EMAIL_AUTH_TYPE + + resp = admin_request.post( + 'user.update_user_attribute', + user_id=sample_user.id, + _data={'mobile_number': ''}, + _expected_status=400 + ) + assert resp['message']['mobile_number'] == ['Invalid phone number: Not enough digits'] From 6332058781bb556d4cdb6962d165ba125c03ec17 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Nov 2017 19:10:49 +0000 Subject: [PATCH 31/35] remove PUT /user/ --- app/models.py | 2 +- app/user/rest.py | 17 ------ tests/app/user/test_rest.py | 112 ------------------------------------ 3 files changed, 1 insertion(+), 130 deletions(-) diff --git a/app/models.py b/app/models.py index bd6436b05..6adb5abb5 100644 --- a/app/models.py +++ b/app/models.py @@ -563,7 +563,7 @@ class Template(db.Model): nullable=False, default=NORMAL ) - + redact_personalisation = association_proxy('template_redacted', 'redact_personalisation') def get_link(self): diff --git a/app/user/rest.py b/app/user/rest.py index acf7e8abb..787c78f44 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -77,23 +77,6 @@ def create_user(): return jsonify(data=user_schema.dump(user_to_create).data), 201 -@user_blueprint.route('/', methods=['PUT']) -def update_user(user_id): - user_to_update = get_user_by_id(user_id=user_id) - req_json = request.get_json() - update_dct, errors = user_schema_load_json.load(req_json) - # TODO don't let password be updated in this PUT method (currently used by the forgot password flow) - pwd = req_json.get('password', None) - if pwd is not None: - if not pwd: - errors.update({'password': ['Invalid data for field']}) - raise InvalidRequest(errors, status_code=400) - else: - reset_failed_login_count(user_to_update) - save_model_user(user_to_update, update_dict=update_dct, pwd=pwd) - return jsonify(data=user_schema.dump(user_to_update).data), 200 - - @user_blueprint.route('/', methods=['POST']) def update_user_attribute(user_id): user_to_update = get_user_by_id(user_id=user_id) diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 880cf2cd1..8c97e5d70 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -208,42 +208,6 @@ def test_cannot_create_user_with_empty_strings(admin_request, notify_db_session) } -def test_put_user(client, sample_service): - """ - Tests PUT endpoint '/' to update a user. - """ - assert User.query.count() == 1 - sample_user = sample_service.users[0] - sample_user.failed_login_count = 1 - new_email = 'new@digital.cabinet-office.gov.uk' - data = { - 'name': sample_user.name, - 'email_address': new_email, - 'mobile_number': sample_user.mobile_number - } - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - resp = client.put( - url_for('user.update_user', user_id=sample_user.id), - data=json.dumps(data), - headers=headers) - assert resp.status_code == 200 - assert User.query.count() == 1 - json_resp = json.loads(resp.get_data(as_text=True)) - assert json_resp['data']['email_address'] == new_email - expected_permissions = default_service_permissions - fetched = json_resp['data'] - - assert str(sample_user.id) == fetched['id'] - assert sample_user.name == fetched['name'] - assert sample_user.mobile_number == fetched['mobile_number'] - assert new_email == fetched['email_address'] - assert sample_user.state == fetched['state'] - assert sorted(expected_permissions) == sorted(fetched['permissions'][str(sample_service.id)]) - # password wasn't updated, so failed_login_count stays the same - assert sample_user.failed_login_count == 1 - - @pytest.mark.parametrize('user_attribute, user_value', [ ('name', 'New User'), ('email_address', 'newuser@mail.com'), @@ -267,63 +231,6 @@ def test_post_user_attribute(client, sample_user, user_attribute, user_value): assert json_resp['data'][user_attribute] == user_value -def test_put_user_update_password(client, sample_service): - """ - Tests PUT endpoint '/' to update a user including their password. - """ - assert User.query.count() == 1 - sample_user = sample_service.users[0] - new_password = '1234567890' - data = { - 'name': sample_user.name, - 'email_address': sample_user.email_address, - 'mobile_number': sample_user.mobile_number, - 'password': new_password - } - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - resp = client.put( - url_for('user.update_user', user_id=sample_user.id), - data=json.dumps(data), - headers=headers) - assert resp.status_code == 200 - assert User.query.count() == 1 - json_resp = json.loads(resp.get_data(as_text=True)) - assert json_resp['data']['password_changed_at'] is not None - data = {'password': new_password} - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - resp = client.post( - url_for('user.verify_user_password', user_id=str(sample_user.id)), - data=json.dumps(data), - headers=headers) - assert resp.status_code == 204 - - -def test_put_user_not_exists(client, sample_user, fake_uuid): - """ - Tests PUT endpoint '/' to update a user doesn't exist. - """ - assert User.query.count() == 1 - new_email = 'new@digital.cabinet-office.gov.uk' - data = {'email_address': new_email} - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - resp = client.put( - url_for('user.update_user', user_id=fake_uuid), - data=json.dumps(data), - headers=headers) - assert resp.status_code == 404 - assert User.query.count() == 1 - user = User.query.filter_by(id=str(sample_user.id)).first() - json_resp = json.loads(resp.get_data(as_text=True)) - assert json_resp['result'] == "error" - assert json_resp['message'] == 'No result found' - - assert user == sample_user - assert user.email_address != new_email - - def test_get_user_by_email(client, sample_service): sample_user = sample_service.users[0] header = create_authorization_header() @@ -578,25 +485,6 @@ def test_update_user_password_saves_correctly(client, sample_service): assert resp.status_code == 204 -def test_update_user_resets_failed_login_count_if_updating_password(client, sample_service): - user = sample_service.users[0] - user.failed_login_count = 1 - - resp = client.put( - url_for('user.update_user', user_id=user.id), - data=json.dumps({ - 'name': user.name, - 'email_address': user.email_address, - 'mobile_number': user.mobile_number, - 'password': 'foo' - }), - headers=[('Content-Type', 'application/json'), create_authorization_header()] - ) - - assert resp.status_code == 200 - assert user.failed_login_count == 0 - - def test_activate_user(admin_request, sample_user): sample_user.state = 'pending' From 9e9642f6e9dca924c4f54d0f8774eea3e2662270 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 10 Nov 2017 12:05:06 +0000 Subject: [PATCH 32/35] Change to fix ip restrictions --- app/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/__init__.py b/app/__init__.py index b93abfe6e..ed6e8e603 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -124,7 +124,7 @@ def register_blueprint(application): application.register_blueprint(sms_callback_blueprint) # inbound sms - receive_notifications_blueprint.before_request(restrict_ip_sms) + receive_notifications_blueprint.before_request(requires_no_auth) application.register_blueprint(receive_notifications_blueprint) notifications_blueprint.before_request(requires_auth) From 28287abbd8c0541494754c0cfe132bf996387b4d Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Fri, 10 Nov 2017 13:49:20 +0000 Subject: [PATCH 33/35] Added test to handle Null template_id There are some Null template_ids in the production database which was causing a failure as the stats_template_usage_by_month has a constraint that it should not be null. This update only adds populated template_ids - Updated Celery task - Add a new test to test for null template_ids and not try to add them to the stats --- app/celery/scheduled_tasks.py | 13 +++--- tests/app/celery/test_scheduled_tasks.py | 57 +++++++++++++++++++++--- 2 files changed, 58 insertions(+), 12 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index e39876df0..82dc30f76 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -414,9 +414,10 @@ def daily_stats_template_usage_by_month(): results = dao_fetch_monthly_historical_stats_by_template() for result in results: - insert_or_update_stats_for_template( - result.template_id, - result.month, - result.year, - result.count - ) + if result.template_id: + insert_or_update_stats_for_template( + result.template_id, + result.month, + result.year, + result.count + ) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index d31e2b5a4..01e40dd95 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -44,16 +44,20 @@ from app.dao.provider_details_dao import ( get_current_provider ) from app.models import ( - Service, Template, - SMS_TYPE, LETTER_TYPE, + MonthlyBilling, + NotificationHistory, + Service, + StatsTemplateUsageByMonth, + Template, JOB_STATUS_READY_TO_SEND, JOB_STATUS_IN_PROGRESS, JOB_STATUS_SENT_TO_DVLA, - NOTIFICATION_PENDING, - NOTIFICATION_CREATED, KEY_TYPE_TEST, - MonthlyBilling, - StatsTemplateUsageByMonth) + LETTER_TYPE, + NOTIFICATION_CREATED, + NOTIFICATION_PENDING, + SMS_TYPE +) from app.utils import get_london_midnight_in_utc from app.v2.errors import JobIncompleteError from tests.app.db import create_notification, create_service, create_template, create_job, create_rate @@ -943,3 +947,44 @@ def test_daily_stats_template_usage_by_month_multiple_runs(notify_db, notify_db_ assert result[3].month == 11 assert result[3].year == 2017 assert result[3].count == 1 + + +def test_dao_fetch_monthly_historical_stats_by_template_null_template_id_not_counted(notify_db, notify_db_session): + notification_history = functools.partial( + create_notification_history, + notify_db, + notify_db_session, + status='delivered' + ) + + template_one = create_sample_template(notify_db, notify_db_session, template_name='1') + history = notification_history(created_at=datetime(2017, 2, 1), sample_template=template_one) + + NotificationHistory.query.filter( + NotificationHistory.id == history.id + ).update( + { + 'template_id': None + } + ) + + daily_stats_template_usage_by_month() + + result = db.session.query( + StatsTemplateUsageByMonth + ).all() + + assert len(result) == 0 + + notification_history(created_at=datetime(2017, 2, 1), sample_template=template_one) + + daily_stats_template_usage_by_month() + + result = db.session.query( + StatsTemplateUsageByMonth + ).order_by( + StatsTemplateUsageByMonth.year, + StatsTemplateUsageByMonth.month + ).all() + + assert len(result) == 1 From fe6bafcfb2b8885449b4482066d791b369eae001 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 10 Nov 2017 14:17:29 +0000 Subject: [PATCH 34/35] Update the from number in the response of post notification. --- app/notifications/validators.py | 2 +- app/v2/notifications/post_notifications.py | 4 ++-- tests/app/notifications/test_validators.py | 2 +- tests/app/v2/notifications/test_post_notifications.py | 1 + 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 4f05977ce..5aee7c55b 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -155,7 +155,7 @@ def check_service_sms_sender_id(service_id, sms_sender_id, notification_type): message = 'sms_sender_id is not a valid option for {} notification'.format(notification_type) raise BadRequestError(message=message) try: - dao_get_service_sms_senders_by_id(service_id, sms_sender_id) + return dao_get_service_sms_senders_by_id(service_id, sms_sender_id).sms_sender except NoResultFound: message = 'sms_sender_id {} does not exist in database for service id {}'\ .format(sms_sender_id, service_id) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 87d55159d..90cd2997f 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -71,7 +71,7 @@ def post_notification(notification_type): check_rate_limiting(authenticated_service, api_user) check_service_email_reply_to_id(str(authenticated_service.id), service_email_reply_to_id, notification_type) - check_service_sms_sender_id(str(authenticated_service.id), service_sms_sender_id, notification_type) + sms_sender = check_service_sms_sender_id(str(authenticated_service.id), service_sms_sender_id, notification_type) template, template_with_content = validate_template( form['template_id'], @@ -98,7 +98,7 @@ def post_notification(notification_type): if notification_type == SMS_TYPE: create_resp_partial = functools.partial( create_post_sms_response_from_notification, - from_number=authenticated_service.get_default_sms_sender() + from_number=sms_sender or authenticated_service.get_default_sms_sender() ) elif notification_type == EMAIL_TYPE: create_resp_partial = functools.partial( diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index ede6d940d..a20251790 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -374,7 +374,7 @@ def test_check_service_sms_sender_id_where_sms_sender_id_is_none(notification_ty def test_check_service_sms_sender_id_where_sms_sender_id_is_found(sample_service): sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456') - assert check_service_sms_sender_id(sample_service.id, sms_sender.id, SMS_TYPE) is None + assert check_service_sms_sender_id(sample_service.id, sms_sender.id, SMS_TYPE) == '123456' def test_check_service_sms_sender_id_where_service_id_is_not_found(sample_service, fake_uuid): diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index d4055df3f..a1746ae06 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -118,6 +118,7 @@ def test_post_sms_notification_returns_201_with_sms_sender_id( notification_to_sms_sender = NotificationSmsSender.query.all() assert len(notification_to_sms_sender) == 1 assert str(notification_to_sms_sender[0].notification_id) == resp_json['id'] + assert resp_json['content']['from_number'] == sms_sender.sms_sender assert notification_to_sms_sender[0].service_sms_sender_id == sms_sender.id mocked.assert_called_once_with([resp_json['id']], queue='send-sms-tasks') From 834eecd0f14b51752f6f16d9d5a3557338a8549b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 10 Nov 2017 15:24:37 +0000 Subject: [PATCH 35/35] make sure you can't edit password --- tests/app/user/test_rest.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 8c97e5d70..00444ea64 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -564,3 +564,13 @@ def test_cannot_update_user_with_mobile_number_as_empty_string(admin_request, sa _expected_status=400 ) assert resp['message']['mobile_number'] == ['Invalid phone number: Not enough digits'] + + +def test_cannot_update_user_password_using_attributes_method(admin_request, sample_user): + resp = admin_request.post( + 'user.update_user_attribute', + user_id=sample_user.id, + _data={'password': 'foo'}, + _expected_status=400 + ) + assert resp['message']['_schema'] == ['Unknown field name password']