From 9e4abbc511e620dd84c2652216cab919059191ec Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 14 Nov 2018 10:16:55 +0000 Subject: [PATCH 1/3] Refactor duplicative methods These methods were both doing the exact same thing. There may well be others in the future that also need to do this thing. --- app/models/service.py | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index 2ff9ad42e..32de656ba 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -58,6 +58,12 @@ class Service(): return self._dict[attr] raise AttributeError('`{}` is not a service attribute'.format(attr)) + def _get_by_id(self, things, id): + try: + return next(thing for thing in things if thing['id'] == id) + except StopIteration: + abort(404) + def update(self, **kwargs): return service_api_client.update_service(self.id, **kwargs) @@ -310,13 +316,7 @@ class Service(): ] def get_template_folder(self, folder_id): - try: - return next( - folder for folder in self.all_template_folders - if folder['id'] == folder_id - ) - except StopIteration: - abort(404) + return self._get_by_id(self.all_template_folders, folder_id) def is_folder_visible(self, template_folder_id, template_type='all'): @@ -374,7 +374,4 @@ class Service(): ) def get_api_key(self, id): - try: - return next(key for key in self.api_keys if key['id'] == id) - except StopIteration: - abort(404) + return self._get_by_id(self.api_keys, id) From f3d1dc4a9c93c466b70979cd8e1426158154e6a8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 15 Nov 2018 17:04:11 +0000 Subject: [PATCH 2/3] =?UTF-8?q?Don=E2=80=99t=20ask=20for=20data=20retentio?= =?UTF-8?q?n=20unless=20channel=20is=20known?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the user is looking at the notifications page for all message types (which is what we show ‘caseworkers’) then it doesn’t make sense to ask the API for the data retention period for that message type (because it will be `None`). Doing so causes the API to return a `404`, which then causes the admin app to return `404`. Passing through `None` as the value of limit days will just cause the API to return everything in the `notifications` table, which is fine for us. --- app/main/views/jobs.py | 8 +++-- tests/app/main/views/test_activity.py | 43 +++++++++++++++++++++++---- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index cf50029f1..107a096e5 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -216,10 +216,12 @@ def get_notifications(service_id, message_type, status_override=None): abort(404) filter_args = parse_filter_args(request.args) filter_args['status'] = set_status_filters(filter_args) + service_data_retention_days = None - service_data_retention_days = service_api_client.get_service_data_retention_by_notification_type( - service_id, message_type - ).get('days_of_retention', current_app.config['ACTIVITY_STATS_LIMIT_DAYS']) + if message_type is not None: + service_data_retention_days = service_api_client.get_service_data_retention_by_notification_type( + service_id, message_type + ).get('days_of_retention', current_app.config['ACTIVITY_STATS_LIMIT_DAYS']) if request.path.endswith('csv') and current_user.has_permissions('view_activity'): return Response( diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index 2a87968d2..f4327a1e8 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -21,10 +21,28 @@ from tests.conftest import ( @pytest.mark.parametrize( - "user,extra_args,expected_update_endpoint,page_title", [ - (active_user_view_permissions, {'message_type': 'email'}, '/email.json', 'Emails'), - (active_user_view_permissions, {'message_type': 'sms'}, '/sms.json', 'Text messages'), - (active_caseworking_user, {}, '.json', 'Sent messages'), + "user,extra_args,expected_update_endpoint,expected_limit_days,page_title", [ + ( + active_user_view_permissions, + {'message_type': 'email'}, + '/email.json', + 7, + 'Emails', + ), + ( + active_user_view_permissions, + {'message_type': 'sms'}, + '/sms.json', + 7, + 'Text messages', + ), + ( + active_caseworking_user, + {}, + '.json', + None, + 'Sent messages', + ), ] ) @pytest.mark.parametrize( @@ -76,6 +94,7 @@ def test_can_show_notifications( user, extra_args, expected_update_endpoint, + expected_limit_days, page_title, status_argument, expected_api_call, @@ -131,7 +150,7 @@ def test_can_show_notifications( assert 'to' not in query_dict mock_get_notifications.assert_called_with( - limit_days=7, + limit_days=expected_limit_days, page=expected_page_argument, service_id=service_one['id'], status=expected_api_call, @@ -149,6 +168,20 @@ def test_can_show_notifications( assert json_content.keys() == {'counts', 'notifications', 'service_data_retention_days'} +def test_can_show_notifications_if_data_retention_not_available( + client_request, + mock_get_notifications, + mock_get_service_statistics, + mock_has_no_jobs, +): + page = client_request.get( + 'main.view_notifications', + service_id=SERVICE_ONE_ID, + status='sending,delivered,failed', + ) + assert page.h1.text.strip() == 'Messages' + + @pytest.mark.parametrize('user, query_parameters, expected_download_link', [ ( active_user_with_permissions, From 230ea013c6fb7407efa40d9e633cd9241a122d38 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 16 Nov 2018 10:14:58 +0000 Subject: [PATCH 3/3] Update domains for recently new organisations --- app/domains.yml | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/app/domains.yml b/app/domains.yml index 9d4ac1244..839e14aa4 100644 --- a/app/domains.yml +++ b/app/domains.yml @@ -423,10 +423,7 @@ ashbournetowncouncil.gov.uk: owner: Ashbourne Town Council crown: false agreement_signed: false -ashfield-dc.gov.uk: - owner: Ashfield District Council - crown: false - agreement_signed: false +ashfield-dc.gov.uk: ashfield.gov.uk ashfield.gov.uk: owner: Ashfield District Council crown: false @@ -1537,7 +1534,7 @@ dorchester-tc.gov.uk: dorsetcc.gov.uk: owner: Dorset County Council crown: false - agreement_signed: false + agreement_signed: true dorsetforyou.gov.uk: owner: Weymouth and Portland Borough Council crown: false @@ -3715,7 +3712,7 @@ rosstc-herefordshire.gov.uk: rother.gov.uk: owner: Rother District Council crown: false - agreement_signed: false + agreement_signed: true rotherham.gov.uk: owner: Rotherham Borough Council crown: false @@ -5120,15 +5117,9 @@ worcester.gov.uk: worcestershire.gov.uk: owner: Worcestershire County Council crown: false - agreement_signed: false -worcestershirehub.gov.uk: - owner: Worcestershire County Council - crown: false - agreement_signed: false -worcestershirets.gov.uk: - owner: Worcestershire County Council - crown: false - agreement_signed: false + agreement_signed: true +worcestershirehub.gov.uk: worcestershire.gov.uk +worcestershirets.gov.uk: worcestershire.gov.uk worcsregservices.gov.uk: owner: Bromsgrove District Council crown: false