From f1ea39d4c07ff2607db5cafc6df6f86588e54ada Mon Sep 17 00:00:00 2001 From: Paul Craig Date: Tue, 22 Nov 2016 17:30:03 +0000 Subject: [PATCH 01/10] Simplify `_filter_query()` function --- app/dao/notifications_dao.py | 15 ++++++++++----- app/notifications/rest.py | 4 ++-- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index a1d15d87a..2da210069 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -296,15 +296,20 @@ def get_notifications_for_service( def _filter_query(query, filter_dict=None): if filter_dict is None: - filter_dict = MultiDict() - else: - filter_dict = MultiDict(filter_dict) - statuses = filter_dict.getlist('status') if 'status' in filter_dict else None + return query + + multidict = MultiDict(filter_dict) + + # filter by status + statuses = multidict.getlist('status') if statuses: query = query.filter(Notification.status.in_(statuses)) - template_types = filter_dict.getlist('template_type') if 'template_type' in filter_dict else None + + # filter by template + template_types = multidict.getlist('template_type') if template_types: query = query.join(Template).filter(Template.template_type.in_(template_types)) + return query diff --git a/app/notifications/rest.py b/app/notifications/rest.py index c8b18da6b..126f0feee 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -170,8 +170,8 @@ def get_notification_by_id(notification_id): def get_all_notifications(): data = notifications_filter_schema.load(request.args).data include_jobs = data.get('include_jobs', False) - page = data['page'] if 'page' in data else 1 - page_size = data['page_size'] if 'page_size' in data else current_app.config.get('PAGE_SIZE') + page = data.get('page', 1) + page_size = data.get('page_size', current_app.config.get('PAGE_SIZE')) limit_days = data.get('limit_days') pagination = notifications_dao.get_notifications_for_service( From 1fce30aaa74eb308f3f939d5efa0b16b0a2a952b Mon Sep 17 00:00:00 2001 From: Paul Craig Date: Thu, 24 Nov 2016 10:33:38 +0000 Subject: [PATCH 02/10] Cost savings The "cost" value was flawed for a couple of reasons. 1. Lots of messages are free, so in those instances the "cost" doesn't tell you anything 2. The query to get the rate was expensive and we don't have an obvious way to get it back very efficiently for large numbers of notifications. So we scrapped it. --- app/models.py | 18 +-------- app/v2/notifications/notification_schemas.py | 4 +- tests/app/test_model.py | 37 ------------------- .../notifications/test_get_notifications.py | 1 - 4 files changed, 2 insertions(+), 58 deletions(-) diff --git a/app/models.py b/app/models.py index b880ccf40..119341265 100644 --- a/app/models.py +++ b/app/models.py @@ -6,7 +6,7 @@ from sqlalchemy.dialects.postgresql import ( UUID, JSON ) -from sqlalchemy import UniqueConstraint, and_, desc +from sqlalchemy import UniqueConstraint, and_ from sqlalchemy.orm import foreign, remote from notifications_utils.recipients import ( validate_email_address, @@ -544,21 +544,6 @@ class Notification(db.Model): if personalisation: self._personalisation = encryption.encrypt(personalisation) - def cost(self): - if not self.sent_by or self.billable_units == 0: - return 0 - - provider_rate = db.session.query( - ProviderRates - ).join(ProviderDetails).filter( - ProviderDetails.identifier == self.sent_by, - ProviderRates.provider_id == ProviderDetails.id - ).order_by( - desc(ProviderRates.valid_from) - ).limit(1).one() - - return float(provider_rate.rate * self.billable_units) - def completed_at(self): if self.status in [ NOTIFICATION_DELIVERED, @@ -591,7 +576,6 @@ class Notification(db.Model): "line_5": None, "line_6": None, "postcode": None, - "cost": self.cost(), "type": self.notification_type, "status": self.status, "template": template_dict, diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 4dfaee7b1..272800d5c 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -57,7 +57,6 @@ get_notification_response = { "line_5": {"type": ["string", "null"]}, "line_6": {"type": ["string", "null"]}, "postcode": {"type": ["string", "null"]}, - "cost": {"type": "number"}, "type": {"enum": ["sms", "letter", "email"]}, "status": {"type": "string"}, "template": template, @@ -69,8 +68,7 @@ get_notification_response = { # technically, all keys are required since we always have all of them "id", "reference", "email_address", "phone_number", "line_1", "line_2", "line_3", "line_4", "line_5", "line_6", "postcode", - "cost", "type", "status", "template", - "created_at", "sent_at", "completed_at" + "type", "status", "template", "created_at", "sent_at", "completed_at" ] } diff --git a/tests/app/test_model.py b/tests/app/test_model.py index f60be4f08..ca6d11f9a 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -1,8 +1,5 @@ import pytest -from datetime import datetime -from sqlalchemy.orm.exc import NoResultFound -from tests.app.conftest import sample_notification, sample_provider_rate from app.models import ( ServiceWhitelist, MOBILE_TYPE, EMAIL_TYPE) @@ -35,37 +32,3 @@ def test_should_build_service_whitelist_from_email_address(email_address): def test_should_not_build_service_whitelist_from_invalid_contact(recipient_type, contact): with pytest.raises(ValueError): ServiceWhitelist.from_string('service_id', recipient_type, contact) - - -@pytest.mark.parametrize('provider, billable_units, expected_cost', [ - ('mmg', 1, 3.5), - ('firetext', 2, 0.025), - ('ses', 0, 0) -]) -def test_calculate_cost_from_notification_billable_units( - notify_db, notify_db_session, provider, billable_units, expected_cost -): - provider_rates = [ - ('mmg', datetime(2016, 7, 1), 1.5), - ('firetext', datetime(2016, 7, 1), 0.0125), - ('mmg', datetime.utcnow(), 3.5), - ] - for provider_identifier, valid_from, rate in provider_rates: - sample_provider_rate( - notify_db, - notify_db_session, - provider_identifier=provider_identifier, - valid_from=valid_from, - rate=rate - ) - - notification = sample_notification(notify_db, notify_db_session, billable_units=billable_units, sent_by=provider) - assert notification.cost() == expected_cost - - -def test_billable_units_without_provider_rates_entry_raises_exception( - notify_db, notify_db_session, sample_provider_rate -): - notification = sample_notification(notify_db, notify_db_session, sent_by='not_a_provider') - with pytest.raises(NoResultFound): - notification.cost() diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 555490638..e042c3e33 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -46,7 +46,6 @@ def test_get_notification_by_id_returns_200( 'line_5': None, 'line_6': None, 'postcode': None, - 'cost': sample_notification.cost(), 'type': '{}'.format(sample_notification.notification_type), 'status': '{}'.format(sample_notification.status), 'template': expected_template_response, From effbd315e068240be696f091dba71128cab0ca50 Mon Sep 17 00:00:00 2001 From: Paul Craig Date: Wed, 23 Nov 2016 11:44:38 +0000 Subject: [PATCH 03/10] Create 'v2' get notifications route Return multiple notifications for a service. Choosing a page_size or a page_number is no longer allowed. Instead, there is a `next` link included with will return the next {default_page_size} notifications in the sequence. Query parameters accepted are: - template_type: filter by specific template types - status: filter by specific statuses - older_than: return a chronological list of notifications older than this one. The notification with the id that is passed in is _not_ returned. Note that both `template_type` and `status` can accept multiple parameters. Thus it is possible to call `/v2/notifications?status=created&status=sending&status=delivered` --- app/dao/notifications_dao.py | 8 +++++- app/schemas.py | 1 + app/v2/notifications/get_notifications.py | 34 +++++++++++++++++++---- 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 2da210069..fedf8b46d 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -262,7 +262,8 @@ def get_notifications_for_service( key_type=None, personalisation=False, include_jobs=False, - include_from_test_key=False + include_from_test_key=False, + older_than=None ): if page_size is None: page_size = current_app.config['PAGE_SIZE'] @@ -273,6 +274,11 @@ def get_notifications_for_service( days_ago = date.today() - timedelta(days=limit_days) filters.append(func.date(Notification.created_at) >= days_ago) + if older_than is not None: + older_than_created_at = db.session.query( + Notification.created_at).filter(Notification.id == older_than).as_scalar() + filters.append(Notification.created_at < older_than_created_at) + if not include_jobs or (key_type and key_type != KEY_TYPE_NORMAL): filters.append(Notification.job_id.is_(None)) diff --git a/app/schemas.py b/app/schemas.py index 610683a2d..0c913427e 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -468,6 +468,7 @@ class NotificationsFilterSchema(ma.Schema): limit_days = fields.Int(required=False) include_jobs = fields.Boolean(required=False) include_from_test_key = fields.Boolean(required=False) + older_than = fields.UUID(required=False) @pre_load def handle_multidict(self, in_data): diff --git a/app/v2/notifications/get_notifications.py b/app/v2/notifications/get_notifications.py index 405713dd4..77ba0379c 100644 --- a/app/v2/notifications/get_notifications.py +++ b/app/v2/notifications/get_notifications.py @@ -1,7 +1,8 @@ -from flask import jsonify +from flask import jsonify, request, url_for from app import api_user from app.dao import notifications_dao +from app.schemas import notifications_filter_schema from app.v2.notifications import notification_blueprint @@ -14,9 +15,30 @@ def get_notification_by_id(id): return jsonify(notification.serialize()), 200 -@notification_blueprint.route("/", methods=['GET']) +@notification_blueprint.route("", methods=['GET']) def get_notifications(): - # validate notifications request arguments - # fetch all notifications - # return notifications_response schema - pass + data = notifications_filter_schema.load(request.args).data + + paginated_notifications = notifications_dao.get_notifications_for_service( + str(api_user.service_id), + filter_dict=data, + key_type=api_user.key_type, + personalisation=True, + older_than=data.get('older_than') + ) + + def _build_links(notifications): + _links = { + 'current': url_for(".get_notifications", **request.args.to_dict(flat=False)), + } + + if len(notifications): + next_query_params = dict(request.args.to_dict(flat=False), older_than=notifications[-1].id) + _links['next'] = url_for(".get_notifications", **next_query_params) + + return _links + + return jsonify( + notifications=[notification.serialize() for notification in paginated_notifications.items], + links=_build_links(paginated_notifications.items) + ), 200 From 640f51fc0af5c74bf6cee85d04aa5387a3fe51d3 Mon Sep 17 00:00:00 2001 From: Paul Craig Date: Thu, 24 Nov 2016 10:31:14 +0000 Subject: [PATCH 04/10] Add tests for getting back lists of notifications --- app/models.py | 2 +- .../notifications/test_get_notifications.py | 228 +++++++++++++++++- 2 files changed, 228 insertions(+), 2 deletions(-) diff --git a/app/models.py b/app/models.py index 119341265..ba707563c 100644 --- a/app/models.py +++ b/app/models.py @@ -550,7 +550,7 @@ class Notification(db.Model): NOTIFICATION_FAILED, NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_TEMPORARY_FAILURE, - NOTIFICATION_PERMANENT_FAILURE + NOTIFICATION_PERMANENT_FAILURE, ]: return self.updated_at.strftime(DATETIME_FORMAT) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index e042c3e33..af785b6b5 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -3,7 +3,10 @@ import pytest from app import DATETIME_FORMAT from tests import create_authorization_header -from tests.app.conftest import sample_notification as create_sample_notification +from tests.app.conftest import ( + sample_notification as create_sample_notification, + sample_template as create_sample_template +) @pytest.mark.parametrize('billable_units, provider', [ @@ -55,3 +58,226 @@ def test_get_notification_by_id_returns_200( } assert json_response == expected_response + + +def test_get_all_notifications_returns_200(client, notify_db, notify_db_session): + notifications = [create_sample_notification(notify_db, notify_db_session) for _ in range(2)] + notification = notifications[-1] + + auth_header = create_authorization_header(service_id=notification.service_id) + response = client.get( + path='/v2/notifications', + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 200 + assert response.headers['Content-type'] == "application/json" + assert json_response['links']['current'] == "/v2/notifications" + assert 'next' in json_response['links'].keys() + assert len(json_response['notifications']) == 2 + + assert json_response['notifications'][0]['id'] == str(notification.id) + assert json_response['notifications'][0]['status'] == "created" + assert json_response['notifications'][0]['template'] == { + 'id': str(notification.template.id), + 'uri': notification.template.get_link(), + 'version': 1 + } + assert json_response['notifications'][0]['phone_number'] == "+447700900855" + assert json_response['notifications'][0]['type'] == "sms" + + +def test_get_all_notifications_no_notifications_if_no_notificatons(client, sample_service): + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.get( + path='/v2/notifications', + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 200 + assert response.headers['Content-type'] == "application/json" + assert json_response['links']['current'] == "/v2/notifications" + assert 'next' not in json_response['links'].keys() + assert len(json_response['notifications']) == 0 + + +def test_get_all_notifications_filter_by_template_type(client, notify_db, notify_db_session): + email_template = create_sample_template(notify_db, notify_db_session, template_type="email") + sms_template = create_sample_template(notify_db, notify_db_session, template_type="sms") + + notification = create_sample_notification( + notify_db, notify_db_session, template=email_template, to_field="don.draper@scdp.biz") + create_sample_notification(notify_db, notify_db_session, template=sms_template) + + auth_header = create_authorization_header(service_id=notification.service_id) + response = client.get( + path='/v2/notifications?template_type=email', + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 200 + assert response.headers['Content-type'] == "application/json" + assert json_response['links']['current'] == "/v2/notifications?template_type=email" + assert 'next' in json_response['links'].keys() + assert len(json_response['notifications']) == 1 + + assert json_response['notifications'][0]['id'] == str(notification.id) + assert json_response['notifications'][0]['status'] == "created" + assert json_response['notifications'][0]['template'] == { + 'id': str(email_template.id), + 'uri': email_template.get_link(), + 'version': 1 + } + assert json_response['notifications'][0]['email_address'] == "don.draper@scdp.biz" + assert json_response['notifications'][0]['type'] == "email" + + +def test_get_all_notifications_filter_by_single_status(client, notify_db, notify_db_session): + notification = create_sample_notification(notify_db, notify_db_session, status="pending") + create_sample_notification(notify_db, notify_db_session) + + auth_header = create_authorization_header(service_id=notification.service_id) + response = client.get( + path='/v2/notifications?status=pending', + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 200 + assert response.headers['Content-type'] == "application/json" + assert json_response['links']['current'] == "/v2/notifications?status=pending" + assert 'next' in json_response['links'].keys() + assert len(json_response['notifications']) == 1 + + assert json_response['notifications'][0]['id'] == str(notification.id) + assert json_response['notifications'][0]['status'] == "pending" + + +def test_get_all_notifications_filter_by_multiple_statuses(client, notify_db, notify_db_session): + notifications = [ + create_sample_notification(notify_db, notify_db_session, status=_status) + for _status in ["created", "pending", "sending"] + ] + failed_notification = create_sample_notification(notify_db, notify_db_session, status="permanent-failure") + + auth_header = create_authorization_header(service_id=notifications[0].service_id) + response = client.get( + path='/v2/notifications?status=created&status=pending&status=sending', + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 200 + assert response.headers['Content-type'] == "application/json" + assert json_response['links']['current'] == "/v2/notifications?status=created&status=pending&status=sending" + assert 'next' in json_response['links'].keys() + assert len(json_response['notifications']) == 3 + + returned_notification_ids = [_n['id'] for _n in json_response['notifications']] + for _id in [_notification.id for _notification in notifications]: + assert str(_id) in returned_notification_ids + + assert failed_notification.id not in returned_notification_ids + + +def test_get_all_notifications_filter_by_failed_status(client, notify_db, notify_db_session): + pass + + +def test_get_all_notifications_filter_by_id(client, notify_db, notify_db_session): + older_notification = create_sample_notification(notify_db, notify_db_session) + newer_notification = create_sample_notification(notify_db, notify_db_session) + + auth_header = create_authorization_header(service_id=newer_notification.service_id) + response = client.get( + path='/v2/notifications?older_than={}'.format(newer_notification.id), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 200 + assert response.headers['Content-type'] == "application/json" + assert json_response['links']['current'] == "/v2/notifications?older_than={}".format(newer_notification.id) + assert 'next' in json_response['links'].keys() + assert len(json_response['notifications']) == 1 + + assert json_response['notifications'][0]['id'] == str(older_notification.id) + + +def test_get_all_notifications_filter_by_id_no_notifications_if_nonexistent_id(client, notify_db, notify_db_session): + notification = create_sample_notification(notify_db, notify_db_session) + + auth_header = create_authorization_header(service_id=notification.service_id) + response = client.get( + path='/v2/notifications?older_than=dd4b8b9d-d414-4a83-9256-580046bf18f9', + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 200 + assert response.headers['Content-type'] == "application/json" + assert json_response['links']['current'] == "/v2/notifications?older_than=dd4b8b9d-d414-4a83-9256-580046bf18f9" + assert 'next' not in json_response['links'].keys() + assert len(json_response['notifications']) == 0 + + +def test_get_all_notifications_filter_by_id_no_notifications_if_last_notification(client, notify_db, notify_db_session): + notification = create_sample_notification(notify_db, notify_db_session) + + auth_header = create_authorization_header(service_id=notification.service_id) + response = client.get( + path='/v2/notifications?older_than={}'.format(notification.id), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 200 + assert response.headers['Content-type'] == "application/json" + assert json_response['links']['current'] == "/v2/notifications?older_than={}".format(notification.id) + assert 'next' not in json_response['links'].keys() + assert len(json_response['notifications']) == 0 + + +def test_get_all_notifications_filter_multiple_query_parameters(client, notify_db, notify_db_session): + email_template = create_sample_template(notify_db, notify_db_session, template_type="email") + + # this is the notification we are looking for + older_notification = create_sample_notification( + notify_db, notify_db_session, template=email_template, status="pending") + + # wrong status + create_sample_notification(notify_db, notify_db_session, template=email_template) + # wrong template + create_sample_notification(notify_db, notify_db_session, status="pending") + + # we only want notifications created before this one + newer_notification = create_sample_notification(notify_db, notify_db_session) + + # this notification was created too recently + create_sample_notification(notify_db, notify_db_session, template=email_template, status="pending") + + auth_header = create_authorization_header(service_id=newer_notification.service_id) + response = client.get( + path='/v2/notifications?status=pending&template_type=email&older_than={}'.format(newer_notification.id), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 200 + assert response.headers['Content-type'] == "application/json" + # query parameters aren't returned in order + for url_part in [ + "/v2/notifications?", + "template_type=email", + "status=pending", + "older_than={}".format(newer_notification.id) + ]: + assert url_part in json_response['links']['current'] + + assert 'next' in json_response['links'].keys() + assert len(json_response['notifications']) == 1 + + assert json_response['notifications'][0]['id'] == str(older_notification.id) From 7d009915a478207fc2efa8ce6161eddfd1f7d440 Mon Sep 17 00:00:00 2001 From: Paul Craig Date: Fri, 25 Nov 2016 14:53:21 +0000 Subject: [PATCH 05/10] Group 'completed' notification statuses Some notification statuses assume that a notification has been updated (ie, it cannot have been created in that state). This caused a bug in our sample notification fixture when trying to create a notificaiton in a 'complete' status. This commit groups the completed statuses in a list, the way other statuses have been grouped together so that they're more portable. Also fixed the sample_notification fixture. --- app/models.py | 16 +++++++++------- tests/app/conftest.py | 5 +++-- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/app/models.py b/app/models.py index ba707563c..456fc5122 100644 --- a/app/models.py +++ b/app/models.py @@ -466,6 +466,14 @@ NOTIFICATION_TECHNICAL_FAILURE = 'technical-failure' NOTIFICATION_TEMPORARY_FAILURE = 'temporary-failure' NOTIFICATION_PERMANENT_FAILURE = 'permanent-failure' +NOTIFICATION_STATUS_TYPES_COMPLETED = [ + NOTIFICATION_DELIVERED, + NOTIFICATION_FAILED, + NOTIFICATION_TECHNICAL_FAILURE, + NOTIFICATION_TEMPORARY_FAILURE, + NOTIFICATION_PERMANENT_FAILURE, +] + NOTIFICATION_STATUS_TYPES_BILLABLE = [ NOTIFICATION_SENDING, NOTIFICATION_DELIVERED, @@ -545,13 +553,7 @@ class Notification(db.Model): self._personalisation = encryption.encrypt(personalisation) def completed_at(self): - if self.status in [ - NOTIFICATION_DELIVERED, - NOTIFICATION_FAILED, - NOTIFICATION_TECHNICAL_FAILURE, - NOTIFICATION_TEMPORARY_FAILURE, - NOTIFICATION_PERMANENT_FAILURE, - ]: + if self.status in NOTIFICATION_STATUS_TYPES_COMPLETED: return self.updated_at.strftime(DATETIME_FORMAT) return None diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 76187e6e2..a21584cd3 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -21,7 +21,7 @@ from app.models import ( NotificationStatistics, ServiceWhitelist, KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM, - MOBILE_TYPE, EMAIL_TYPE) + MOBILE_TYPE, EMAIL_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED) from app.dao.users_dao import (save_model_user, create_user_code, create_secret_code) from app.dao.services_dao import (dao_create_service, dao_add_user_to_service) from app.dao.templates_dao import dao_create_template @@ -444,7 +444,8 @@ def sample_notification(notify_db, 'notification_type': template.template_type, 'api_key_id': api_key_id, 'key_type': key_type, - 'sent_by': sent_by + 'sent_by': sent_by, + 'updated_at': created_at if status in NOTIFICATION_STATUS_TYPES_COMPLETED else None } if job_row_number: data['job_row_number'] = job_row_number From 57a0d7295d4fa61cebe396e7c65e0e56dff7365c Mon Sep 17 00:00:00 2001 From: Paul Craig Date: Fri, 25 Nov 2016 14:55:29 +0000 Subject: [PATCH 06/10] Rewrite `failed` statuses There are no more notifications whose statuses are "failed", as the "failed" status has now been replaced with statuses that are more specific about the nature of the failure. However, we still want to be able to filter by failing notifications. (ie "/v2/notifications?status=failed"). Created a `.substitute_status()` method which takes a status string or list of status strings and, if it finds 'failure', replaces it with the other failing status types. This way, calling for nottifications with "?status=failed" is internally treated as "status = ['technical-failure', 'temporary-failure', 'permanent-failure']" --- app/models.py | 50 +++++++++++++++++++++++++++++++++++++++-- tests/app/test_model.py | 35 ++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/app/models.py b/app/models.py index 456fc5122..c24ce7b18 100644 --- a/app/models.py +++ b/app/models.py @@ -466,6 +466,12 @@ NOTIFICATION_TECHNICAL_FAILURE = 'technical-failure' NOTIFICATION_TEMPORARY_FAILURE = 'temporary-failure' NOTIFICATION_PERMANENT_FAILURE = 'permanent-failure' +NOTIFICATION_STATUS_TYPES_FAILED = [ + NOTIFICATION_TECHNICAL_FAILURE, + NOTIFICATION_TEMPORARY_FAILURE, + NOTIFICATION_PERMANENT_FAILURE, +] + NOTIFICATION_STATUS_TYPES_COMPLETED = [ NOTIFICATION_DELIVERED, NOTIFICATION_FAILED, @@ -480,7 +486,7 @@ NOTIFICATION_STATUS_TYPES_BILLABLE = [ NOTIFICATION_FAILED, NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_TEMPORARY_FAILURE, - NOTIFICATION_PERMANENT_FAILURE + NOTIFICATION_PERMANENT_FAILURE, ] NOTIFICATION_STATUS_TYPES = [ @@ -491,7 +497,7 @@ NOTIFICATION_STATUS_TYPES = [ NOTIFICATION_FAILED, NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_TEMPORARY_FAILURE, - NOTIFICATION_PERMANENT_FAILURE + NOTIFICATION_PERMANENT_FAILURE, ] NOTIFICATION_STATUS_TYPES_ENUM = db.Enum(*NOTIFICATION_STATUS_TYPES, name='notify_status_type') @@ -558,6 +564,46 @@ class Notification(db.Model): return None + @staticmethod + def substitute_status(status_or_statuses): + """ + static function that takes a status or list of statuses and substitutes our new failure types if it finds + the deprecated one + + > IN + 'failed' + + < OUT + ['technical-failure', 'temporary-failure', 'permanent-failure'] + + - + + > IN + ['failed', 'created'] + + < OUT + ['technical-failure', 'temporary-failure', 'permanent-failure', 'created'] + + + :param status_or_statuses: a single status or list of statuses + :return: a single status or list with the current failure statuses substituted for 'failure' + """ + + def _substitute_status_str(_status): + return NOTIFICATION_STATUS_TYPES_FAILED if _status == NOTIFICATION_FAILED else _status + + def _substitute_status_seq(_statuses): + if NOTIFICATION_FAILED in _statuses: + _statuses = list(set( + NOTIFICATION_STATUS_TYPES_FAILED + [_s for _s in _statuses if _s != NOTIFICATION_FAILED] + )) + return _statuses + + if isinstance(status_or_statuses, str): + return _substitute_status_str(status_or_statuses) + + return _substitute_status_seq(status_or_statuses) + def serialize(self): template_dict = { diff --git a/tests/app/test_model.py b/tests/app/test_model.py index ca6d11f9a..21b3df4cc 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -2,7 +2,15 @@ import pytest from app.models import ( ServiceWhitelist, - MOBILE_TYPE, EMAIL_TYPE) + Notification, + MOBILE_TYPE, + EMAIL_TYPE, + NOTIFICATION_CREATED, + NOTIFICATION_PENDING, + NOTIFICATION_FAILED, + NOTIFICATION_TECHNICAL_FAILURE, + NOTIFICATION_STATUS_TYPES_FAILED +) @pytest.mark.parametrize('mobile_number', [ @@ -32,3 +40,28 @@ def test_should_build_service_whitelist_from_email_address(email_address): def test_should_not_build_service_whitelist_from_invalid_contact(recipient_type, contact): with pytest.raises(ValueError): ServiceWhitelist.from_string('service_id', recipient_type, contact) + + +@pytest.mark.parametrize('initial_statuses, expected_statuses', [ + # passing in single statuses as strings + (NOTIFICATION_FAILED, NOTIFICATION_STATUS_TYPES_FAILED), + (NOTIFICATION_CREATED, NOTIFICATION_CREATED), + (NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_TECHNICAL_FAILURE), + # passing in lists containing single statuses + ([NOTIFICATION_FAILED], NOTIFICATION_STATUS_TYPES_FAILED), + ([NOTIFICATION_CREATED], [NOTIFICATION_CREATED]), + ([NOTIFICATION_TECHNICAL_FAILURE], [NOTIFICATION_TECHNICAL_FAILURE]), + # passing in lists containing multiple statuses + ([NOTIFICATION_FAILED, NOTIFICATION_CREATED], NOTIFICATION_STATUS_TYPES_FAILED + [NOTIFICATION_CREATED]), + ([NOTIFICATION_CREATED, NOTIFICATION_PENDING], [NOTIFICATION_CREATED, NOTIFICATION_PENDING]), + ([NOTIFICATION_CREATED, NOTIFICATION_TECHNICAL_FAILURE], [NOTIFICATION_CREATED, NOTIFICATION_TECHNICAL_FAILURE]), + # checking we don't end up with duplicates + ( + [NOTIFICATION_FAILED, NOTIFICATION_CREATED, NOTIFICATION_TECHNICAL_FAILURE], + NOTIFICATION_STATUS_TYPES_FAILED + [NOTIFICATION_CREATED] + ), +]) +def test_status_conversion_handles_failed_statuses(initial_statuses, expected_statuses): + converted_statuses = Notification.substitute_status(initial_statuses) + assert len(converted_statuses) == len(expected_statuses) + assert set(converted_statuses) == set(expected_statuses) From df7450698c1c3a8cd9aa8bea1fbc5f505c2d7b50 Mon Sep 17 00:00:00 2001 From: Paul Craig Date: Fri, 25 Nov 2016 14:55:45 +0000 Subject: [PATCH 07/10] Test returning notifications by "?status=failed" Check that all failure states are returned by asking for notifications of type "failure". --- app/dao/notifications_dao.py | 1 + tests/app/dao/test_notification_dao.py | 8 +++--- .../notifications/test_get_notifications.py | 25 ++++++++++++++++++- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index fedf8b46d..86cdda671 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -309,6 +309,7 @@ def _filter_query(query, filter_dict=None): # filter by status statuses = multidict.getlist('status') if statuses: + statuses = Notification.substitute_status(statuses) query = query.filter(Notification.status.in_(statuses)) # filter by template diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index ea7779ecb..fe768631f 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -8,8 +8,6 @@ import pytest from freezegun import freeze_time from sqlalchemy.exc import SQLAlchemyError, IntegrityError -from app import db - from app.models import ( Notification, NotificationHistory, @@ -17,6 +15,7 @@ from app.models import ( NotificationStatistics, TemplateStatistics, NOTIFICATION_STATUS_TYPES, + NOTIFICATION_STATUS_TYPES_FAILED, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST @@ -683,7 +682,10 @@ def test_get_all_notifications_for_job_by_status(notify_db, notify_db_session, s assert len(notifications().items) == len(NOTIFICATION_STATUS_TYPES) for status in NOTIFICATION_STATUS_TYPES: - assert len(notifications(filter_dict={'status': status}).items) == 1 + if status == 'failed': + assert len(notifications(filter_dict={'status': status}).items) == len(NOTIFICATION_STATUS_TYPES_FAILED) + else: + assert len(notifications(filter_dict={'status': status}).items) == 1 assert len(notifications(filter_dict={'status': NOTIFICATION_STATUS_TYPES[:3]}).items) == 3 diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index af785b6b5..423243e6c 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -184,7 +184,30 @@ def test_get_all_notifications_filter_by_multiple_statuses(client, notify_db, no def test_get_all_notifications_filter_by_failed_status(client, notify_db, notify_db_session): - pass + created_notification = create_sample_notification(notify_db, notify_db_session, status="created") + failed_notifications = [ + create_sample_notification(notify_db, notify_db_session, status=_status) + for _status in ["technical-failure", "temporary-failure", "permanent-failure"] + ] + + auth_header = create_authorization_header(service_id=created_notification.service_id) + response = client.get( + path='/v2/notifications?status=failed', + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 200 + assert response.headers['Content-type'] == "application/json" + assert json_response['links']['current'] == "/v2/notifications?status=failed" + assert 'next' in json_response['links'].keys() + assert len(json_response['notifications']) == 3 + + returned_notification_ids = [n['id'] for n in json_response['notifications']] + for _id in [_notification.id for _notification in failed_notifications]: + assert str(_id) in returned_notification_ids + + assert created_notification.id not in returned_notification_ids def test_get_all_notifications_filter_by_id(client, notify_db, notify_db_session): From ffb813bb3f9d1aaf0ae2f372e2c9fe02bf4bd06d Mon Sep 17 00:00:00 2001 From: Paul Craig Date: Fri, 25 Nov 2016 16:46:19 +0000 Subject: [PATCH 08/10] Add get_notifications json schema + tests Add a schema that corresponds to our response for returning lists of notifications, and test with a contract test. --- app/v2/notifications/notification_schemas.py | 30 +++++++++++++++++++ .../public_contracts/test_GET_notification.py | 13 +++++++- 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 272800d5c..934d6d400 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -72,6 +72,36 @@ get_notification_response = { ] } +get_notifications_response = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "GET list of notifications response schema", + "type": "object", + "properties": { + "notifications": { + "type": "array", + "items": { + "type": "object", + "ref": get_notification_response + } + }, + "links": { + "type": "object", + "properties": { + "current": { + "type": "string" + }, + "next": { + "type": "string" + } + }, + "additionalProperties": False, + "required": ["current"] + } + }, + "additionalProperties": False, + "required": ["notifications", "links"] +} + post_sms_request = { "$schema": "http://json-schema.org/draft-04/schema#", "description": "POST sms notification schema", diff --git a/tests/app/public_contracts/test_GET_notification.py b/tests/app/public_contracts/test_GET_notification.py index 0a3adc74c..005f2c93d 100644 --- a/tests/app/public_contracts/test_GET_notification.py +++ b/tests/app/public_contracts/test_GET_notification.py @@ -1,7 +1,7 @@ from . import return_json_from_response, validate_v0, validate from app.models import ApiKey, KEY_TYPE_NORMAL from app.dao.api_key_dao import save_model_api_key -from app.v2.notifications.notification_schemas import get_notification_response +from app.v2.notifications.notification_schemas import get_notification_response, get_notifications_response from tests import create_authorization_header @@ -16,6 +16,8 @@ def _get_notification(client, notification, url): return client.get(url, headers=[auth_header]) +# v2 + def test_get_v2_sms_contract(client, sample_notification): response_json = return_json_from_response(_get_notification( client, sample_notification, '/v2/notifications/{}'.format(sample_notification.id) @@ -30,6 +32,15 @@ def test_get_v2_email_contract(client, sample_email_notification): validate(response_json, get_notification_response) +def test_get_v2_notifications_contract(client, sample_notification): + response_json = return_json_from_response(_get_notification( + client, sample_notification, '/v2/notifications' + )) + validate(response_json, get_notifications_response) + + +# v0 + def test_get_api_sms_contract(client, sample_notification): response_json = return_json_from_response(_get_notification( client, sample_notification, '/notifications/{}'.format(sample_notification.id) From 268e19bb769efe6ad3bb67f99fecb3be213f3359 Mon Sep 17 00:00:00 2001 From: Paul Craig Date: Fri, 25 Nov 2016 16:58:46 +0000 Subject: [PATCH 09/10] Fix ServiceWhiteList __repr__ Apparently this code was unreachable. --- app/models.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/models.py b/app/models.py index c24ce7b18..9f3a583a9 100644 --- a/app/models.py +++ b/app/models.py @@ -178,9 +178,8 @@ class ServiceWhitelist(db.Model): else: return instance - def __repr__(self): - return 'Recipient {} of type: {}'.format(self.recipient, - self.recipient_type) + def __repr__(self): + return 'Recipient {} of type: {}'.format(self.recipient, self.recipient_type) class ApiKey(db.Model, Versioned): From 9b1375ba84b78d22d7af5a3e0fa1cebc553fcfe0 Mon Sep 17 00:00:00 2001 From: Paul Craig Date: Mon, 28 Nov 2016 11:13:11 +0000 Subject: [PATCH 10/10] URLs in API responses have full URL --- app/models.py | 7 ++++++- app/v2/notifications/get_notifications.py | 4 ++-- .../notifications/test_get_notifications.py | 19 ++++++++++--------- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/app/models.py b/app/models.py index 9f3a583a9..e5785582f 100644 --- a/app/models.py +++ b/app/models.py @@ -284,7 +284,12 @@ class Template(db.Model): def get_link(self): # TODO: use "/v2/" route once available - return url_for("template.get_template_by_id_and_service_id", service_id=self.service_id, template_id=self.id) + return url_for( + "template.get_template_by_id_and_service_id", + service_id=self.service_id, + template_id=self.id, + _external=True + ) class TemplateHistory(db.Model): diff --git a/app/v2/notifications/get_notifications.py b/app/v2/notifications/get_notifications.py index 77ba0379c..274df52d7 100644 --- a/app/v2/notifications/get_notifications.py +++ b/app/v2/notifications/get_notifications.py @@ -29,12 +29,12 @@ def get_notifications(): def _build_links(notifications): _links = { - 'current': url_for(".get_notifications", **request.args.to_dict(flat=False)), + 'current': url_for(".get_notifications", _external=True, **request.args.to_dict(flat=False)), } if len(notifications): next_query_params = dict(request.args.to_dict(flat=False), older_than=notifications[-1].id) - _links['next'] = url_for(".get_notifications", **next_query_params) + _links['next'] = url_for(".get_notifications", _external=True, **next_query_params) return _links diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 423243e6c..3c1b93134 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -73,7 +73,7 @@ def test_get_all_notifications_returns_200(client, notify_db, notify_db_session) assert response.status_code == 200 assert response.headers['Content-type'] == "application/json" - assert json_response['links']['current'] == "/v2/notifications" + assert json_response['links']['current'].endswith("/v2/notifications") assert 'next' in json_response['links'].keys() assert len(json_response['notifications']) == 2 @@ -98,7 +98,7 @@ def test_get_all_notifications_no_notifications_if_no_notificatons(client, sampl assert response.status_code == 200 assert response.headers['Content-type'] == "application/json" - assert json_response['links']['current'] == "/v2/notifications" + assert json_response['links']['current'].endswith("/v2/notifications") assert 'next' not in json_response['links'].keys() assert len(json_response['notifications']) == 0 @@ -120,7 +120,7 @@ def test_get_all_notifications_filter_by_template_type(client, notify_db, notify assert response.status_code == 200 assert response.headers['Content-type'] == "application/json" - assert json_response['links']['current'] == "/v2/notifications?template_type=email" + assert json_response['links']['current'].endswith("/v2/notifications?template_type=email") assert 'next' in json_response['links'].keys() assert len(json_response['notifications']) == 1 @@ -148,7 +148,7 @@ def test_get_all_notifications_filter_by_single_status(client, notify_db, notify assert response.status_code == 200 assert response.headers['Content-type'] == "application/json" - assert json_response['links']['current'] == "/v2/notifications?status=pending" + assert json_response['links']['current'].endswith("/v2/notifications?status=pending") assert 'next' in json_response['links'].keys() assert len(json_response['notifications']) == 1 @@ -172,7 +172,7 @@ def test_get_all_notifications_filter_by_multiple_statuses(client, notify_db, no assert response.status_code == 200 assert response.headers['Content-type'] == "application/json" - assert json_response['links']['current'] == "/v2/notifications?status=created&status=pending&status=sending" + assert json_response['links']['current'].endswith("/v2/notifications?status=created&status=pending&status=sending") assert 'next' in json_response['links'].keys() assert len(json_response['notifications']) == 3 @@ -199,7 +199,7 @@ def test_get_all_notifications_filter_by_failed_status(client, notify_db, notify assert response.status_code == 200 assert response.headers['Content-type'] == "application/json" - assert json_response['links']['current'] == "/v2/notifications?status=failed" + assert json_response['links']['current'].endswith("/v2/notifications?status=failed") assert 'next' in json_response['links'].keys() assert len(json_response['notifications']) == 3 @@ -223,7 +223,7 @@ def test_get_all_notifications_filter_by_id(client, notify_db, notify_db_session assert response.status_code == 200 assert response.headers['Content-type'] == "application/json" - assert json_response['links']['current'] == "/v2/notifications?older_than={}".format(newer_notification.id) + assert json_response['links']['current'].endswith("/v2/notifications?older_than={}".format(newer_notification.id)) assert 'next' in json_response['links'].keys() assert len(json_response['notifications']) == 1 @@ -242,7 +242,8 @@ def test_get_all_notifications_filter_by_id_no_notifications_if_nonexistent_id(c assert response.status_code == 200 assert response.headers['Content-type'] == "application/json" - assert json_response['links']['current'] == "/v2/notifications?older_than=dd4b8b9d-d414-4a83-9256-580046bf18f9" + assert json_response['links']['current'].endswith( + "/v2/notifications?older_than=dd4b8b9d-d414-4a83-9256-580046bf18f9") assert 'next' not in json_response['links'].keys() assert len(json_response['notifications']) == 0 @@ -259,7 +260,7 @@ def test_get_all_notifications_filter_by_id_no_notifications_if_last_notificatio assert response.status_code == 200 assert response.headers['Content-type'] == "application/json" - assert json_response['links']['current'] == "/v2/notifications?older_than={}".format(notification.id) + assert json_response['links']['current'].endswith("/v2/notifications?older_than={}".format(notification.id)) assert 'next' not in json_response['links'].keys() assert len(json_response['notifications']) == 0