From b1cfa8942ad14f004a28f759b920c3743998cda3 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 18 Jul 2018 10:54:20 +0100 Subject: [PATCH] Add one_off filter when getting all notifications for a service Added the option to filter by one_off messages to the DAO function `get_notifications_for_service`. Previously, one-off notifications were not returned - this has changed so that the default is for one-off notifications to be returned. Also simplified the `include_jobs` filter for this function. The DAO function gets used in 3 places - for the V1 and V2 API endpoints, which will now start to return one-off messages. It also gets used by the admin app which needs to pass in `include_one_off=False` to the `get_all_notifications_for_service` where we don't want one-off notifications to show, such as the API message log page. --- app/dao/notifications_dao.py | 12 +++++++----- app/schemas.py | 1 + app/service/rest.py | 4 +++- .../dao/notification_dao/test_notification_dao.py | 15 +++++++++++++++ tests/app/db.py | 6 ++++-- tests/app/service/test_rest.py | 10 ++++++++-- 6 files changed, 38 insertions(+), 10 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 8eac58fc4..dce08f132 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -31,7 +31,6 @@ from app.models import ( Service, Template, TemplateHistory, - KEY_TYPE_NORMAL, KEY_TYPE_TEST, LETTER_TYPE, NOTIFICATION_CREATED, @@ -245,7 +244,8 @@ def get_notifications_for_service( include_jobs=False, include_from_test_key=False, older_than=None, - client_reference=None + client_reference=None, + include_one_off=True ): if page_size is None: page_size = current_app.config['PAGE_SIZE'] @@ -260,9 +260,11 @@ def get_notifications_for_service( 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): - # we can't say "job_id == None" here, because letters sent via the API still have a job_id :( - filters.append(Notification.api_key_id != None) # noqa + if not include_jobs: + filters.append(Notification.job_id == None) # noqa + + if not include_one_off: + filters.append(Notification.created_by_id == None) # noqa if key_type is not None: filters.append(Notification.key_type == key_type) diff --git a/app/schemas.py b/app/schemas.py index de226f215..db21fef8e 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -593,6 +593,7 @@ class NotificationsFilterSchema(ma.Schema): older_than = fields.UUID(required=False) format_for_csv = fields.String() to = fields.String() + include_one_off = fields.Boolean(required=False) @pre_load def handle_multidict(self, in_data): diff --git a/app/service/rest.py b/app/service/rest.py index 60f5e0358..8eb157b98 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -335,6 +335,7 @@ def get_all_notifications_for_service(service_id): limit_days = data.get('limit_days') include_jobs = data.get('include_jobs', True) include_from_test_key = data.get('include_from_test_key', False) + include_one_off = data.get('include_one_off', True) pagination = notifications_dao.get_notifications_for_service( service_id, @@ -343,7 +344,8 @@ def get_all_notifications_for_service(service_id): page_size=page_size, limit_days=limit_days, include_jobs=include_jobs, - include_from_test_key=include_from_test_key + include_from_test_key=include_from_test_key, + include_one_off=include_one_off ) kwargs = request.args.to_dict() kwargs['service_id'] = service_id diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 0980f021b..3e36c697c 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -957,6 +957,21 @@ def test_should_return_notifications_excluding_jobs_by_default(sample_template, assert exclude_jobs_manually[0].id == without_job.id +def test_should_return_notifications_including_one_offs_by_default(sample_user, sample_template): + create_notification(sample_template, one_off=True, created_by_id=sample_user.id) + not_one_off = create_notification(sample_template) + + exclude_one_offs = get_notifications_for_service(sample_template.service_id, include_one_off=False).items + assert len(exclude_one_offs) == 1 + assert exclude_one_offs[0].id == not_one_off.id + + include_one_offs_manually = get_notifications_for_service(sample_template.service_id, include_one_off=True).items + assert len(include_one_offs_manually) == 2 + + include_one_offs_by_default = get_notifications_for_service(sample_template.service_id).items + assert len(include_one_offs_by_default) == 2 + + def test_get_notifications_created_by_api_or_csv_are_returned_correctly_excluding_test_key_notifications( notify_db, notify_db_session, diff --git a/tests/app/db.py b/tests/app/db.py index bf5ac243c..b42203a6b 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -176,7 +176,8 @@ def create_notification( normalised_to=None, one_off=False, sms_sender_id=None, - reply_to_text=None + reply_to_text=None, + created_by_id=None ): if created_at is None: created_at = datetime.utcnow() @@ -221,7 +222,8 @@ def create_notification( 'international': international, 'phone_prefix': phone_prefix, 'normalised_to': normalised_to, - 'reply_to_text': reply_to_text + 'reply_to_text': reply_to_text, + 'created_by_id': created_by_id } notification = Notification(**data) dao_create_notification(notification) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index f3a90b937..ac4fb027a 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1384,15 +1384,21 @@ def test_get_all_notifications_for_service_including_ones_made_by_jobs( def test_get_only_api_created_notifications_for_service( admin_request, sample_job, - sample_template + sample_template, + sample_user, ): + # notification sent as a job create_notification(sample_template, job=sample_job) + # notification sent as a one-off + create_notification(sample_template, one_off=True, created_by_id=sample_user.id) + # notification sent via API without_job = create_notification(sample_template) resp = admin_request.get( 'service.get_all_notifications_for_service', service_id=sample_template.service_id, - include_jobs=False + include_jobs=False, + include_one_off=False ) assert len(resp['notifications']) == 1 assert resp['notifications'][0]['id'] == str(without_job.id)