From f030f6b37d8b64558243187e39981926d995eb30 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Fri, 15 Sep 2017 11:01:58 +0100 Subject: [PATCH 1/8] Bump utils to 21.2.0 This will stop logging the request, as it is logged by nginx-access logs --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 0b1bd52a6..a1157fe49 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,4 +26,4 @@ notifications-python-client==4.4.0 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@21.0.0#egg=notifications-utils==21.0.0 +git+https://github.com/alphagov/notifications-utils.git@21.2.0#egg=notifications-utils==21.2.0 From 4312789b60cdf0fd6d34fcc048bca12da3613959 Mon Sep 17 00:00:00 2001 From: chrisw Date: Tue, 19 Sep 2017 13:32:42 +0100 Subject: [PATCH 2/8] Add a non-GOV.UK banner option for email branding Added extra radio button for 'org_banner' option Updated service setting template to display appropriate text when option is selected Updated tests to also accomodate new radio option --- app/main/forms.py | 3 ++- app/templates/views/service-settings.html | 2 ++ tests/app/main/views/test_service_settings.py | 4 ++++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/main/forms.py b/app/main/forms.py index 1bdbb2ba6..74356b0a5 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -511,7 +511,8 @@ class ServiceBrandingOrg(Form): choices=[ ('govuk', 'GOV.UK only'), ('both', 'GOV.UK and organisation'), - ('org', 'Organisation only') + ('org', 'Organisation only'), + ('org_banner', 'Organisation banner') ], validators=[ DataRequired() diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index b945e24ce..873a590fb 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -187,6 +187,8 @@ GOV.UK and {{ organisation.name if organisation else None }} {% elif current_service.branding == 'org' %} Only {{ organisation.name if organisation else None }} + {% elif current_service.branding == 'org_banner' %} + Only {{ organisation.name if organisation else None }} banner {% endif %} {% endcall %} {{ edit_field('Change', url_for('.service_set_branding_and_org', service_id=current_service.id)) }} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index ec1e8156e..a11bcd0ec 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -922,10 +922,12 @@ def test_should_show_branding( assert page.find('input', attrs={"id": "branding_type-0"})['value'] == 'govuk' assert page.find('input', attrs={"id": "branding_type-1"})['value'] == 'both' assert page.find('input', attrs={"id": "branding_type-2"})['value'] == 'org' + assert page.find('input', attrs={"id": "branding_type-3"})['value'] == 'org_banner' assert 'checked' in page.find('input', attrs={"id": "branding_type-0"}).attrs assert 'checked' not in page.find('input', attrs={"id": "branding_type-1"}).attrs assert 'checked' not in page.find('input', attrs={"id": "branding_type-2"}).attrs + assert 'checked' not in page.find('input', attrs={"id": "branding_type-3"}).attrs app.organisations_client.get_organisations.assert_called_once_with() app.service_api_client.get_service.assert_called_once_with(service_one['id']) @@ -945,10 +947,12 @@ def test_should_show_organisations( assert page.find('input', attrs={"id": "branding_type-0"})['value'] == 'govuk' assert page.find('input', attrs={"id": "branding_type-1"})['value'] == 'both' assert page.find('input', attrs={"id": "branding_type-2"})['value'] == 'org' + assert page.find('input', attrs={"id": "branding_type-3"})['value'] == 'org_banner' assert 'checked' in page.find('input', attrs={"id": "branding_type-0"}).attrs assert 'checked' not in page.find('input', attrs={"id": "branding_type-1"}).attrs assert 'checked' not in page.find('input', attrs={"id": "branding_type-2"}).attrs + assert 'checked' not in page.find('input', attrs={"id": "branding_type-3"}).attrs app.organisations_client.get_organisations.assert_called_once_with() app.service_api_client.get_service.assert_called_once_with(service_one['id']) From f8fab35ee7c056119a25f136f31e281f09d085b0 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 20 Sep 2017 14:38:15 +0100 Subject: [PATCH 3/8] show letters as accepted, not created or sending, on the api page --- app/main/views/api_keys.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index ba6a83a18..ca7329d9d 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -13,13 +13,18 @@ from app.notify_client.api_key_api_client import KEY_TYPE_NORMAL, KEY_TYPE_TEST, def api_integration(service_id): return render_template( 'views/api/index.html', - api_notifications=notification_api_client.get_notifications_for_service( + api_notifications=map_letters_to_accepted(notification_api_client.get_notifications_for_service( service_id=service_id, include_jobs=False, include_from_test_key=True - ) + )) ) +def map_letters_to_accepted(notifications): + for notification in notifications['notifications']: + if notification['notification_type'] == 'letter' and notification['status'] in ('created', 'sending'): + notification['status'] = 'accepted' + return notifications @main.route("/services//api/documentation") @login_required From 03397f416e9b23d6941c694ddfdc3e73d40dd6fb Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 20 Sep 2017 16:02:15 +0100 Subject: [PATCH 4/8] move status mapping logic to the api client also added tests :angel: --- app/main/views/api_keys.py | 11 +------- app/notify_client/notification_api_client.py | 11 ++++++++ tests/__init__.py | 6 +++-- .../notify_client/test_notification_client.py | 25 +++++++++++++++++++ 4 files changed, 41 insertions(+), 12 deletions(-) diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index ca7329d9d..68a93f8d2 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -13,18 +13,9 @@ from app.notify_client.api_key_api_client import KEY_TYPE_NORMAL, KEY_TYPE_TEST, def api_integration(service_id): return render_template( 'views/api/index.html', - api_notifications=map_letters_to_accepted(notification_api_client.get_notifications_for_service( - service_id=service_id, - include_jobs=False, - include_from_test_key=True - )) + api_notifications=notification_api_client.get_api_notifications_for_service(service_id) ) -def map_letters_to_accepted(notifications): - for notification in notifications['notifications']: - if notification['notification_type'] == 'letter' and notification['status'] in ('created', 'sending'): - notification['status'] = 'accepted' - return notifications @main.route("/services//api/documentation") @login_required diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index f39ff33d0..cec5346cc 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -66,3 +66,14 @@ class NotificationApiClient(NotifyAdminAPIClient): def get_notification(self, service_id, notification_id): return self.get(url='/service/{}/notifications/{}'.format(service_id, notification_id)) + + def get_api_notifications_for_service(self, service_id): + ret = self.get_notifications_for_service(service_id, include_jobs=False, include_from_test_key=True) + return self.map_letters_to_accepted(ret) + + @staticmethod + def map_letters_to_accepted(notifications): + for notification in notifications['notifications']: + if notification['notification_type'] == 'letter' and notification['status'] in ('created', 'sending'): + notification['status'] = 'accepted' + return notifications diff --git a/tests/__init__.py b/tests/__init__.py index 629fedb6d..301b6dc0d 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -266,6 +266,7 @@ def notification_json( 'service': service_id, 'template_version': template['version'], 'personalisation': personalisation or {}, + 'notification_type': 'sms', } for i in range(rows)], 'total': rows, 'page_size': 50, @@ -281,7 +282,8 @@ def single_notification_json( status=None, sent_at=None, created_at=None, - updated_at=None + updated_at=None, + notification_type='sms' ): if template is None: template = template_json(service_id, str(generate_uuid())) @@ -310,7 +312,7 @@ def single_notification_json( 'id': '29441662-17ce-4ffe-9502-fcaed73b2826', 'template': template, 'job_row_number': 0, - 'notification_type': 'sms', + 'notification_type': notification_type, 'api_key': None, 'job': job_payload, 'sent_by': 'mmg' diff --git a/tests/app/notify_client/test_notification_client.py b/tests/app/notify_client/test_notification_client.py index 4f9750a05..4f7ddd5da 100644 --- a/tests/app/notify_client/test_notification_client.py +++ b/tests/app/notify_client/test_notification_client.py @@ -1,6 +1,11 @@ +import uuid + import pytest + from app.notify_client.notification_api_client import NotificationApiClient +from tests import single_notification_json, notification_json + @pytest.mark.parametrize("arguments,expected_call", [ ( @@ -55,3 +60,23 @@ def test_get_notification(mocker): mock_get.assert_called_once_with( url='/service/foo/notifications/bar' ) + + +def test_get_api_notifications_changes_letter_statuses(mocker): + service_id = str(uuid.uuid4()) + sms_notification = single_notification_json(service_id, notification_type='sms', status='created') + email_notification = single_notification_json(service_id, notification_type='email', status='created') + letter_notification = single_notification_json(service_id, notification_type='letter', status='created') + notis = notification_json(service_id=service_id, rows=0) + notis['notifications'] = [sms_notification, email_notification, letter_notification] + + mock_post = mocker.patch('app.notify_client.notification_api_client.NotificationApiClient.get', return_value=notis) + + ret = NotificationApiClient().get_api_notifications_for_service(service_id) + + assert ret['notifications'][0]['notification_type'] == 'sms' + assert ret['notifications'][1]['notification_type'] == 'email' + assert ret['notifications'][2]['notification_type'] == 'letter' + assert ret['notifications'][0]['status'] == 'created' + assert ret['notifications'][1]['status'] == 'created' + assert ret['notifications'][2]['status'] == 'accepted' From 6d421a04247499e5611b8dda68ae3ffa98252c72 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 20 Sep 2017 16:23:18 +0100 Subject: [PATCH 5/8] Add Biotechnology and Biological Sciences Research Council to email domain list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > BBSRC is an executive non-departmental public body, sponsored by the > Department for Business, Energy & Industrial Strategy. – https://www.gov.uk/government/organisations/biotechnology-biological-sciences-research-council --- app/config.py | 1 + tests/app/main/test_validators.py | 1 + 2 files changed, 2 insertions(+) diff --git a/app/config.py b/app/config.py index 7a2412e5e..2543c7a99 100644 --- a/app/config.py +++ b/app/config.py @@ -87,6 +87,7 @@ class Config(object): r"bl\.uk", r"stfc\.ac\.uk", r"wmfs\.net", + r"bbsrc\.ac\.uk", ] LOGO_UPLOAD_BUCKET_NAME = 'public-logos-local' diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index fd6caa69b..8c823aa70 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -96,6 +96,7 @@ def _gen_mock_field(x): 'test@bl.uk', 'test@stfc.ac.uk', 'test@wmfs.net', + 'test@bbsrc.ac.uk', ]) def test_valid_list_of_white_list_email_domains( client, From d01c397bb45fcaa52e44cfa49b204bfac5ec458a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 21 Sep 2017 10:41:33 +0100 Subject: [PATCH 6/8] Allow creation of an organisation without a logo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now we have the org banner branding, not all organisations need a logo. So it shouldn’t be an error to not provide one. Depends on: - [ ] https://github.com/alphagov/notifications-api/pull/1265 --- app/main/views/organisations.py | 4 ++- app/templates/components/page-footer.html | 5 ++- .../views/organisations/manage-org.html | 3 +- tests/app/main/views/test_organisations.py | 35 ++++++++++++++----- 4 files changed, 32 insertions(+), 15 deletions(-) diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index 0fe976752..c7d41a38e 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -71,7 +71,9 @@ def manage_org(logo=None): return redirect( url_for('.manage_org', logo=upload_filename)) - logo = persist_logo(logo, session["user_id"]) + if logo: + logo = persist_logo(logo, session["user_id"]) + delete_temp_files_created_by(session["user_id"]) if org: diff --git a/app/templates/components/page-footer.html b/app/templates/components/page-footer.html index 74b847a0c..5dcc69e4f 100644 --- a/app/templates/components/page-footer.html +++ b/app/templates/components/page-footer.html @@ -6,13 +6,12 @@ secondary_link=False, secondary_link_text=None, delete_link=False, - delete_link_text="delete", - button_disabled=False + delete_link_text="delete" ) %}
{% if button_text %} - + {% endif %} {% if back_link %} {{ back_link_text }} diff --git a/app/templates/views/organisations/manage-org.html b/app/templates/views/organisations/manage-org.html index 3457230f4..c0385bbf9 100644 --- a/app/templates/views/organisations/manage-org.html +++ b/app/templates/views/organisations/manage-org.html @@ -26,14 +26,13 @@
{{textbox(form.name)}}
-
{{textbox(form.colour, width='1-4')}} +
{{textbox(form.colour, width='1-4')}}
{{ page_footer( 'Save', back_link=url_for('.organisations', organisation_id=organisation.id if organisation else 'None'), back_link_text='Back to organisation selection', - button_disabled=True if not logo else False ) }}
diff --git a/tests/app/main/views/test_organisations.py b/tests/app/main/views/test_organisations.py index 82a6cdee6..270185e74 100644 --- a/tests/app/main/views/test_organisations.py +++ b/tests/app/main/views/test_organisations.py @@ -113,20 +113,11 @@ def test_manage_orgs_shows_correct_org_info(request_get_manage_org_with_org): def test_manage_orgs_does_not_show_data_for_new_org(request_get_manage_org_without_org): - assert request_get_manage_org_without_org.select_one('div.page-footer input.button').has_attr('disabled') assert request_get_manage_org_without_org.select_one('#logo-img > img') is None assert request_get_manage_org_without_org.select_one('#name').attrs.get('value') == '' assert request_get_manage_org_without_org.select_one('#colour').attrs.get('value') == '' -def test_save_is_enabled_when_logo_is_set(request_get_manage_org_with_org): - assert request_get_manage_org_with_org.select_one('div.page-footer input.button').has_attr('disabled') is False - - -def test_save_is_disabled_when_logo_is_not_set(request_get_manage_org_without_org): - assert request_get_manage_org_without_org.select_one('div.page-footer input.button').has_attr('disabled') - - @pytest.fixture def request_post_manage_org_redirect(logged_in_platform_admin_client, mocker, fake_uuid): with logged_in_platform_admin_client.session_transaction() as session: @@ -262,3 +253,29 @@ def test_create_new_organisation_when_organisation_saved(logged_in_platform_admi name=new_org['name'], colour=new_org['colour'] ) + + +def test_create_new_organisation_without_logo(logged_in_platform_admin_client, mocker, fake_uuid): + + new_org = {'logo': None, 'colour': 'red', 'name': 'new name'} + + mocked_new_org = mocker.patch('app.organisations_client.create_organisation') + mock_persist = mocker.patch('app.main.views.organisations.persist_logo') + mocker.patch('app.main.views.organisations.delete_temp_files_created_by') + + logged_in_platform_admin_client.post( + url_for('.manage_org'), + content_type='multipart/form-data', + data={ + 'colour': new_org['colour'], + 'name': new_org['name'], + } + ) + + assert mocked_new_org.called + assert mocked_new_org.call_args == call( + logo=new_org['logo'], + name=new_org['name'], + colour=new_org['colour'] + ) + assert mock_persist.call_args_list == [] From d5ff02e5f7f59720ba53dca84db7b17274cbb0ef Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Thu, 21 Sep 2017 12:08:30 +0100 Subject: [PATCH 7/8] 77-78 for Legal Aid Agency --- app/templates/views/signedout.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index a21f7bbb9..3c0f0b143 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -117,12 +117,12 @@
-
77
+
78
services
-
37
+
38
departments
From 033872c34ee0c8b69a389f226935cb5c2c3e0e4a Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Thu, 21 Sep 2017 15:10:42 +0100 Subject: [PATCH 8/8] And now to 79 --- app/templates/views/signedout.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index 3c0f0b143..5b368988e 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -117,7 +117,7 @@
-
78
+
79
services