From b36d0ff5523067cb847ba31deaf34efbb8b0afd2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 23 Sep 2016 09:43:25 +0100 Subject: [PATCH 1/5] Make indentation more sensible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Starting arguments on their own line and putting the closing parenthesis on it’s own line because any subsequent changes to the arguments diff cleanly (ie without touching any other lines). --- app/dao/notifications_dao.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index fa65e8a72..2453db6c5 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -228,14 +228,16 @@ def get_notifications(filter_dict=None): @statsd(namespace="dao") -def get_notifications_for_service(service_id, - filter_dict=None, - page=1, - page_size=None, - limit_days=None, - key_type=None, - personalisation=False, - include_jobs=False): +def get_notifications_for_service( + service_id, + filter_dict=None, + page=1, + page_size=None, + limit_days=None, + key_type=None, + personalisation=False, + include_jobs=False +): if page_size is None: page_size = current_app.config['PAGE_SIZE'] From f9f3bb8370c3e24a5fee7ea026fdf37179a6a9ce Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 23 Sep 2016 10:27:10 +0100 Subject: [PATCH 2/5] Make DAO optionally return test key notifications Developers need visibility of what their integration is doing within the app. This includes notifications sent with a test key. This commit adds an optional, defaults-to-false parameter to include notifications sent from a test API key when getting notifications. --- app/dao/notifications_dao.py | 5 +++-- tests/app/dao/test_notification_dao.py | 8 ++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 2453db6c5..c289f60ed 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -236,7 +236,8 @@ def get_notifications_for_service( limit_days=None, key_type=None, personalisation=False, - include_jobs=False + include_jobs=False, + include_from_test_key=False ): if page_size is None: page_size = current_app.config['PAGE_SIZE'] @@ -252,7 +253,7 @@ def get_notifications_for_service( if key_type is not None: filters.append(Notification.key_type == key_type) - else: + elif not include_from_test_key: filters.append(Notification.key_type != KEY_TYPE_TEST) query = Notification.query.filter(*filters) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 39c91912c..97885f50a 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1000,11 +1000,15 @@ def test_get_notifications_created_by_api_or_csv_are_returned_correctly_excludin all_notifications = Notification.query.all() assert len(all_notifications) == 4 - # returns all API derived notifications + # returns all real API derived notifications all_notifications = get_notifications_for_service(sample_service.id).items assert len(all_notifications) == 2 - # all notifications including jobs + # returns all API derived notifications, including those created with test key + all_notifications = get_notifications_for_service(sample_service.id, include_from_test_key=True).items + assert len(all_notifications) == 3 + + # all real notifications including jobs all_notifications = get_notifications_for_service(sample_service.id, limit_days=1, include_jobs=True).items assert len(all_notifications) == 3 From d7bb83fadfa1fddfa80f6b193438c90ec853925e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 23 Sep 2016 10:35:31 +0100 Subject: [PATCH 3/5] Optionally get notifications created w/ test key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is only for the method that the admin app uses; it doesn’t affect the public get notifications endpoint. --- app/schemas.py | 1 + app/service/rest.py | 5 ++++- tests/app/service/test_rest.py | 25 +++++++++++++++++++++---- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index ccabfc9d6..bb4aca8e5 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -422,6 +422,7 @@ class NotificationsFilterSchema(ma.Schema): page_size = fields.Int(required=False) limit_days = fields.Int(required=False) include_jobs = fields.Boolean(required=False) + include_from_test_key = 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 011977a84..d50326805 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -214,6 +214,7 @@ def get_all_notifications_for_service(service_id): page_size = data['page_size'] if 'page_size' in data else current_app.config.get('PAGE_SIZE') limit_days = data.get('limit_days') include_jobs = data.get('include_jobs', True) + include_from_test_key = data.get('include_from_test_key', False) pagination = notifications_dao.get_notifications_for_service( service_id, @@ -221,7 +222,9 @@ def get_all_notifications_for_service(service_id): page=page, page_size=page_size, limit_days=limit_days, - include_jobs=include_jobs) + include_jobs=include_jobs, + include_from_test_key=include_from_test_key + ) kwargs = request.args.to_dict() kwargs['service_id'] = service_id return jsonify( diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index ac8c58e3f..6c5287058 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -16,6 +16,7 @@ from tests.app.conftest import ( sample_user as create_sample_user, sample_notification as create_sample_notification, sample_notification_with_job) +from app.models import KEY_TYPE_TEST def test_get_service_list(notify_api, service_factory): @@ -1037,23 +1038,39 @@ def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notif assert response.status_code == 200 +@pytest.mark.parametrize( + 'include_from_test_key, expected_count_of_notifications', + [ + (False, 2), + (True, 3) + ] +) def test_get_all_notifications_for_service_including_ones_made_by_jobs( notify_api, notify_db, notify_db_session, - sample_service): + sample_service, + include_from_test_key, + expected_count_of_notifications +): with notify_api.test_request_context(), notify_api.test_client() as client: with_job = sample_notification_with_job(notify_db, notify_db_session, service=sample_service) without_job = create_sample_notification(notify_db, notify_db_session, service=sample_service) + from_test_api_key = create_sample_notification( + notify_db, notify_db_session, service=sample_service, key_type=KEY_TYPE_TEST + ) auth_header = create_authorization_header() response = client.get( - path='/service/{}/notifications'.format(sample_service.id), - headers=[auth_header]) + path='/service/{}/notifications?include_from_test_key={}'.format( + sample_service.id, include_from_test_key + ), + headers=[auth_header] + ) resp = json.loads(response.get_data(as_text=True)) - assert len(resp['notifications']) == 2 + assert len(resp['notifications']) == expected_count_of_notifications assert resp['notifications'][0]['to'] == with_job.to assert resp['notifications'][1]['to'] == without_job.to assert response.status_code == 200 From ba71079f22800f9ed0ce992652d9a17ea0af94cb Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 23 Sep 2016 10:35:46 +0100 Subject: [PATCH 4/5] Reduce indentation --- tests/app/service/test_rest.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 6c5287058..e3a4ea141 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1046,12 +1046,12 @@ def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notif ] ) def test_get_all_notifications_for_service_including_ones_made_by_jobs( - notify_api, - notify_db, - notify_db_session, - sample_service, - include_from_test_key, - expected_count_of_notifications + notify_api, + notify_db, + notify_db_session, + sample_service, + include_from_test_key, + expected_count_of_notifications ): with notify_api.test_request_context(), notify_api.test_client() as client: with_job = sample_notification_with_job(notify_db, notify_db_session, service=sample_service) From 21f3448fbc1fd3703759654614fe2213913cd2fc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 23 Sep 2016 10:36:28 +0100 Subject: [PATCH 5/5] Use client fixture --- tests/app/service/test_rest.py | 37 +++++++++++++++++----------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index e3a4ea141..179d9383a 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1046,34 +1046,33 @@ def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notif ] ) def test_get_all_notifications_for_service_including_ones_made_by_jobs( - notify_api, + client, notify_db, notify_db_session, sample_service, include_from_test_key, expected_count_of_notifications ): - with notify_api.test_request_context(), notify_api.test_client() as client: - with_job = sample_notification_with_job(notify_db, notify_db_session, service=sample_service) - without_job = create_sample_notification(notify_db, notify_db_session, service=sample_service) - from_test_api_key = create_sample_notification( - notify_db, notify_db_session, service=sample_service, key_type=KEY_TYPE_TEST - ) + with_job = sample_notification_with_job(notify_db, notify_db_session, service=sample_service) + without_job = create_sample_notification(notify_db, notify_db_session, service=sample_service) + from_test_api_key = create_sample_notification( + notify_db, notify_db_session, service=sample_service, key_type=KEY_TYPE_TEST + ) - auth_header = create_authorization_header() + auth_header = create_authorization_header() - response = client.get( - path='/service/{}/notifications?include_from_test_key={}'.format( - sample_service.id, include_from_test_key - ), - headers=[auth_header] - ) + response = client.get( + path='/service/{}/notifications?include_from_test_key={}'.format( + sample_service.id, include_from_test_key + ), + headers=[auth_header] + ) - resp = json.loads(response.get_data(as_text=True)) - assert len(resp['notifications']) == expected_count_of_notifications - assert resp['notifications'][0]['to'] == with_job.to - assert resp['notifications'][1]['to'] == without_job.to - assert response.status_code == 200 + resp = json.loads(response.get_data(as_text=True)) + assert len(resp['notifications']) == expected_count_of_notifications + assert resp['notifications'][0]['to'] == with_job.to + assert resp['notifications'][1]['to'] == without_job.to + assert response.status_code == 200 def test_get_only_api_created_notifications_for_service(