From 68292d2299ef7aa9ab98e6fa53a3a694bf37122c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 16 Mar 2018 17:27:47 +0000 Subject: [PATCH 1/4] Add endpoints to serve the agreement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rather than making users contact us to get the agreement, we should just let them download it, when we know which version to send them. This commit adds two endpoints: - one to serve a page which links to the agreement - one to serve the agreement itself These pages are not linked to anywhere because the underlying files don’t exist yet. So I haven’t bothered putting real content on the page yet either. I imagine the deploy sequence will be: 1. Upload the files to the buckets in each environment 2. Deploy this code through each enviroment, checking the links work 3. Make another PR to start linking to the endpoints added by this commit --- app/config.py | 6 ++ app/domains.yml | 1 + app/main/__init__.py | 3 +- app/main/s3_client.py | 20 +++++ app/main/views/agreement.py | 26 +++++++ app/templates/views/agreement.html | 27 +++++++ app/utils.py | 6 ++ tests/app/main/views/test_agreement.py | 103 +++++++++++++++++++++++++ 8 files changed, 191 insertions(+), 1 deletion(-) create mode 100644 app/main/views/agreement.py create mode 100644 app/templates/views/agreement.html create mode 100644 tests/app/main/views/test_agreement.py diff --git a/app/config.py b/app/config.py index 4f824040d..bec56ba49 100644 --- a/app/config.py +++ b/app/config.py @@ -69,6 +69,7 @@ class Config(object): STATSD_PORT = 8125 NOTIFY_ENVIRONMENT = 'development' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-local' + MOU_BUCKET_NAME = 'local-mou' ROUTE_SECRET_KEY_1 = os.environ.get('ROUTE_SECRET_KEY_1', '') ROUTE_SECRET_KEY_2 = os.environ.get('ROUTE_SECRET_KEY_2', '') CHECK_PROXY_HEADER = False @@ -82,6 +83,7 @@ class Development(Config): STATSD_ENABLED = False CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-tools' + MOU_BUCKET_NAME = 'notify.tools-mou' ADMIN_CLIENT_SECRET = 'dev-notify-secret-key' API_HOST_NAME = 'http://localhost:6011' @@ -98,6 +100,7 @@ class Test(Development): WTF_CSRF_ENABLED = False CSV_UPLOAD_BUCKET_NAME = 'test-notifications-csv-upload' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-test' + MOU_BUCKET_NAME = 'test-mou' NOTIFY_ENVIRONMENT = 'test' API_HOST_NAME = 'http://you-forgot-to-mock-an-api-call-to' TEMPLATE_PREVIEW_API_HOST = 'http://localhost:9999' @@ -109,6 +112,7 @@ class Preview(Config): STATSD_ENABLED = True CSV_UPLOAD_BUCKET_NAME = 'preview-notifications-csv-upload' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-preview' + MOU_BUCKET_NAME = 'notify.works-mou' NOTIFY_ENVIRONMENT = 'preview' CHECK_PROXY_HEADER = True @@ -120,6 +124,7 @@ class Staging(Config): STATSD_ENABLED = True CSV_UPLOAD_BUCKET_NAME = 'staging-notify-csv-upload' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-staging' + MOU_BUCKET_NAME = 'staging-notify.works-mou' NOTIFY_ENVIRONMENT = 'staging' CHECK_PROXY_HEADER = True @@ -131,6 +136,7 @@ class Live(Config): STATSD_ENABLED = True CSV_UPLOAD_BUCKET_NAME = 'live-notifications-csv-upload' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-production' + MOU_BUCKET_NAME = 'notifications.service.gov.uk-mou' NOTIFY_ENVIRONMENT = 'live' CHECK_PROXY_HEADER = False diff --git a/app/domains.yml b/app/domains.yml index 15e368867..823337680 100644 --- a/app/domains.yml +++ b/app/domains.yml @@ -21,6 +21,7 @@ marinemanagement.org.uk: agreement_signed: true cabinet-office.gov.uk: owner: Cabinet Office + crown: true agreement_signed: true cica.gsi.gov.uk: cica.gov.uk cica.gov.uk: diff --git a/app/main/__init__.py b/app/main/__init__.py index a255c4600..7510e236a 100644 --- a/app/main/__init__.py +++ b/app/main/__init__.py @@ -32,5 +32,6 @@ from app.main.views import ( # noqa conversation, organisations, notifications, - inbound_number + inbound_number, + agreement, ) diff --git a/app/main/s3_client.py b/app/main/s3_client.py index a3cd1a691..cbcbb0357 100644 --- a/app/main/s3_client.py +++ b/app/main/s3_client.py @@ -61,6 +61,26 @@ def s3download(service_id, upload_id): return contents +def get_mou(organisation_is_crown): + bucket = current_app.config['MOU_BUCKET_NAME'] + filename = 'crown.pdf' if organisation_is_crown else 'non-crown.pdf' + attachment_filename = 'GOV.UK Notify data sharing and financial agreement{}.pdf'.format( + '' if organisation_is_crown else ' (non-crown)' + ) + try: + key = get_s3_object(bucket, filename) + return { + 'filename_or_fp': key.get()['Body'], + 'attachment_filename': attachment_filename, + 'as_attachment': True, + } + except botocore.exceptions.ClientError as exception: + current_app.logger.error("Unable to download s3 file {}/{}".format( + bucket, filename + )) + raise exception + + def upload_logo(filename, filedata, region, user_id): upload_file_name = LOGO_LOCATION_STRUCTURE.format( temp=TEMP_TAG.format(user_id=user_id), diff --git a/app/main/views/agreement.py b/app/main/views/agreement.py new file mode 100644 index 000000000..9985bd032 --- /dev/null +++ b/app/main/views/agreement.py @@ -0,0 +1,26 @@ +from flask import render_template, send_file +from flask_login import login_required + +from app.main import main +from app.main.views.sub_navigation_dictionaries import features_nav +from app.main.s3_client import get_mou +from app.utils import AgreementInfo + + + +@main.route('/agreement') +@login_required +def agreement(): + return render_template( + 'views/agreement.html', + crown_status=AgreementInfo.from_current_user().crown_status_or_404, + navigation_links=features_nav(), + ) + + +@main.route('/agreement.pdf') +@login_required +def download_agreement(): + return send_file(**get_mou( + AgreementInfo.from_current_user().crown_status_or_404 + )) diff --git a/app/templates/views/agreement.html b/app/templates/views/agreement.html new file mode 100644 index 000000000..9a1c0e5af --- /dev/null +++ b/app/templates/views/agreement.html @@ -0,0 +1,27 @@ +{% extends "withoutnav_template.html" %} +{% from "components/sub-navigation.html" import sub_navigation %} + +{% block per_page_title %} + GOV.UK Notify data sharing and financial agreement +{% endblock %} + +{% block maincolumn_content %} + +
+
+ {{ sub_navigation(navigation_links) }} +
+
+ +

+ GOV.UK Notify data sharing and financial agreement +

+ +

+ Download. +

+ +
+
+ +{% endblock %} diff --git a/app/utils.py b/app/utils.py index 75a5a2138..5466416ef 100644 --- a/app/utils.py +++ b/app/utils.py @@ -464,6 +464,12 @@ class AgreementInfo: else: return 'Can’t tell' + @property + def crown_status_or_404(self): + if self.crown_status is None: + abort(404) + return self.crown_status + def as_request_for_agreement(self, with_owner=False): if with_owner and self.owner: return ( diff --git a/tests/app/main/views/test_agreement.py b/tests/app/main/views/test_agreement.py new file mode 100644 index 000000000..1376e8770 --- /dev/null +++ b/tests/app/main/views/test_agreement.py @@ -0,0 +1,103 @@ +from io import BytesIO + +import pytest +from flask import url_for +from tests.conftest import active_user_with_permissions + + +class _MockS3Object(): + + def __init__(self, data=None): + self.data = data or b'' + + def get(self): + return {'Body': BytesIO(self.data)} + + +@pytest.mark.parametrize('email_address, expected_status', [ + ('test@cabinet-office.gov.uk', 200), + ('test@aylesburytowncouncil.gov.uk', 200), + ('test@unknown.gov.uk', 404), +]) +def test_show_agreement_page( + client_request, + mocker, + fake_uuid, + email_address, + expected_status, +): + user = active_user_with_permissions(fake_uuid) + user.email_address = email_address + mocker.patch('app.user_api_client.get_user', return_value=user) + client_request.get( + 'main.agreement', + _expected_status=expected_status, + ) + + +@pytest.mark.parametrize('email_address, expected_file_fetched, expected_file_served', [ + ( + 'test@cabinet-office.gov.uk', + 'crown.pdf', + 'GOV.UK Notify data sharing and financial agreement.pdf', + ), + ( + 'test@aylesburytowncouncil.gov.uk', + 'non-crown.pdf', + 'GOV.UK Notify data sharing and financial agreement (non-crown).pdf', + ), +]) +def test_downloading_agreement( + logged_in_client, + mocker, + fake_uuid, + email_address, + expected_file_fetched, + expected_file_served, +): + mock_get_s3_object = mocker.patch( + 'app.main.s3_client.get_s3_object', + return_value=_MockS3Object(b'foo') + ) + user = active_user_with_permissions(fake_uuid) + user.email_address = email_address + mocker.patch('app.user_api_client.get_user', return_value=user) + response = logged_in_client.get(url_for('main.download_agreement')) + assert response.status_code == 200 + assert response.get_data() == b'foo' + assert response.headers['Content-Type'] == 'application/pdf' + assert response.headers['Content-Disposition'] == ( + 'attachment; filename="{}"'.format(expected_file_served) + ) + mock_get_s3_object.assert_called_once_with('test-mou', expected_file_fetched) + + +def test_agreement_cant_be_downloaded_unknown_crown_status( + logged_in_client, + mocker, + fake_uuid, +): + mock_get_s3_object = mocker.patch( + 'app.main.s3_client.get_s3_object', + return_value=_MockS3Object() + ) + user = active_user_with_permissions(fake_uuid) + user.email_address = 'test@unknown.gov.uk' + mocker.patch('app.user_api_client.get_user', return_value=user) + response = logged_in_client.get(url_for('main.download_agreement')) + assert response.status_code == 404 + assert mock_get_s3_object.call_args_list == [] + + +def test_agreement_requires_login( + client, + mocker, +): + mock_get_s3_object = mocker.patch( + 'app.main.s3_client.get_s3_object', + return_value=_MockS3Object() + ) + response = client.get(url_for('main.download_agreement')) + assert response.status_code == 302 + assert response.location == 'http://localhost/sign-in?next=%2Fagreement.pdf' + assert mock_get_s3_object.call_args_list == [] From 0d6d4e461a8930d63d708f64387ece7bb83bffe2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 27 Mar 2018 11:34:12 +0100 Subject: [PATCH 2/4] Add wording to download page --- app/templates/views/agreement.html | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/app/templates/views/agreement.html b/app/templates/views/agreement.html index 9a1c0e5af..de005d9bc 100644 --- a/app/templates/views/agreement.html +++ b/app/templates/views/agreement.html @@ -2,7 +2,7 @@ {% from "components/sub-navigation.html" import sub_navigation %} {% block per_page_title %} - GOV.UK Notify data sharing and financial agreement + Download the GOV.UK Notify data sharing and financial agreement {% endblock %} {% block maincolumn_content %} @@ -14,11 +14,21 @@

- GOV.UK Notify data sharing and financial agreement + Download the GOV.UK Notify data sharing and financial agreement

- Download. + This agreement needs to be signed by someone who has the authority to do so on behalf of your whole organisation. Typically this is a director of digital or head of finance. +

+

+ Return a signed copy to notify-support@digital.cabinet-office.gov.uk +

+

+ The agreement contains commercially sensitive information.
+ Do not share it more widely than you need to. +

+

+ Download the agreement.

From c7ab9f7f1af4f604088aeca7e02f360ccda3a43f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 27 Mar 2018 11:13:09 +0100 Subject: [PATCH 3/4] Link to the page to download the agreement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We prefer people downloading the agreement if they can. If we don’t know which agreement they should be using (ie we don’t know their crown status) then we fall back to having them contact us. --- app/templates/views/terms-of-use.html | 21 ++++---------- app/utils.py | 40 ++++++++++++++++++++++++++- tests/app/main/views/test_index.py | 29 +++++++++++++++++-- 3 files changed, 71 insertions(+), 19 deletions(-) diff --git a/app/templates/views/terms-of-use.html b/app/templates/views/terms-of-use.html index f90530c14..0902beeb2 100644 --- a/app/templates/views/terms-of-use.html +++ b/app/templates/views/terms-of-use.html @@ -20,21 +20,12 @@ Terms of use These terms apply to your service’s use of GOV.UK Notify. You must be the service manager to accept them.

- {% if agreement_info.agreement_signed %} -

Your organisation ({{ agreement_info.owner }}) has already accepted the GOV.UK Notify data sharing and financial agreement.

- {% else %} -

- Your organisation - {% if agreement_info.owner %} - ({{ agreement_info.owner }}) - must also accept our data sharing and financial agreement. - Contact us to get a copy. - {% else %} - must also accept our data sharing and financial agreement. - Contact us to get a copy. - {% endif %} -

- {% endif %} +

+ {{ agreement_info.as_terms_of_use_paragraph( + download_link=url_for('.agreement'), + contact_link=url_for('.feedback', ticket_type='ask-question-give-feedback', body='agreement-with-owner') + )}} +

When using Notify

You must:

diff --git a/app/utils.py b/app/utils.py index 5466416ef..8f5719529 100644 --- a/app/utils.py +++ b/app/utils.py @@ -15,7 +15,15 @@ import dateutil import pyexcel import pytz import yaml -from flask import abort, current_app, redirect, request, session, url_for +from flask import ( + Markup, + abort, + current_app, + redirect, + request, + session, + url_for, +) from flask_login import current_user from notifications_utils.recipients import RecipientCSV from notifications_utils.template import ( @@ -464,6 +472,36 @@ class AgreementInfo: else: return 'Can’t tell' + def as_terms_of_use_paragraph(self, **kwargs): + return Markup(self._as_terms_of_use_paragraph(**kwargs)) + + def _as_terms_of_use_paragraph(self, download_link, contact_link): + + if self.agreement_signed: + return ( + 'Your organisation ({}) has already accepted the ' + 'GOV.UK Notify data sharing and financial ' + 'agreement.'.format(self.owner) + ) + + if self.crown_status is not None: + return (( + '{} Download a copy.' + ).format(self._acceptance_required, download_link)) + + return (( + '{} Contact us to get a copy.' + ).format(self._acceptance_required, contact_link)) + + @property + def _acceptance_required(self): + return ( + 'Your organisation {} must also accept our data sharing ' + 'and financial agreement.'.format( + '({})'.format(self.owner) if self.owner else '', + ) + ) + @property def crown_status_or_404(self): if self.crown_status is None: diff --git a/tests/app/main/views/test_index.py b/tests/app/main/views/test_index.py index 4ee404698..c4d64a524 100644 --- a/tests/app/main/views/test_index.py +++ b/tests/app/main/views/test_index.py @@ -1,3 +1,5 @@ +from functools import partial + import pytest from bs4 import BeautifulSoup from flask import url_for @@ -102,13 +104,19 @@ def test_terms_is_generic_if_user_is_not_logged_in( ) -@pytest.mark.parametrize('email_address, expected_terms_paragraph, expected_pricing_paragraph', [ +@pytest.mark.parametrize(( + 'email_address,' + 'expected_terms_paragraph,' + 'expected_terms_link,' + 'expected_pricing_paragraph' +), [ ( 'test@cabinet-office.gov.uk', ( 'Your organisation (Cabinet Office) has already accepted ' 'the GOV.UK Notify data sharing and financial agreement.' ), + None, ( 'Contact us to get a copy of the agreement ' '(Cabinet Office has already accepted it).' @@ -118,8 +126,12 @@ def test_terms_is_generic_if_user_is_not_logged_in( 'test@aylesburytowncouncil.gov.uk', ( 'Your organisation (Aylesbury Town Council) must also ' - 'accept our data sharing and financial agreement. Contact ' - 'us to get a copy.' + 'accept our data sharing and financial agreement. Download ' + 'a copy.' + ), + partial( + url_for, + 'main.agreement', ), ( 'Contact us to get a copy of the agreement ' @@ -132,6 +144,12 @@ def test_terms_is_generic_if_user_is_not_logged_in( 'Your organisation must also accept our data sharing and ' 'financial agreement. Contact us to get a copy.' ), + partial( + url_for, + 'main.feedback', + ticket_type='ask-question-give-feedback', + body='agreement-with-owner', + ), ( 'Contact us to get a copy of the agreement or find out if ' 'we already have one in place with your organisation.' @@ -144,6 +162,7 @@ def test_terms_tells_logged_in_users_what_we_know_about_their_agreement( client_request, email_address, expected_terms_paragraph, + expected_terms_link, expected_pricing_paragraph, ): user = active_user_with_permissions(fake_uuid) @@ -152,4 +171,8 @@ def test_terms_tells_logged_in_users_what_we_know_about_their_agreement( terms_page = client_request.get('main.terms') pricing_page = client_request.get('main.pricing') assert normalize_spaces(terms_page.select('main p')[1].text) == expected_terms_paragraph + if expected_terms_link: + assert terms_page.select_one('main p a')['href'] == expected_terms_link() + else: + assert not terms_page.select_one('main p').select('a') assert normalize_spaces(pricing_page.select('main p')[-1].text) == expected_pricing_paragraph From dca5546cbd0ce490a00bc52841b24bfa109a3173 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 28 Mar 2018 12:30:16 +0100 Subject: [PATCH 4/4] Only offer agreement download to non-crown for now MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don’t have the crown agreement in a nice downloadable format at the moment. --- app/domains.yml | 6 +++++- app/main/views/agreement.py | 3 +-- app/utils.py | 4 ++-- tests/app/main/views/test_agreement.py | 6 +++--- tests/app/main/views/test_index.py | 17 +++++++++++++++++ 5 files changed, 28 insertions(+), 8 deletions(-) diff --git a/app/domains.yml b/app/domains.yml index 823337680..8303cbc2f 100644 --- a/app/domains.yml +++ b/app/domains.yml @@ -159,7 +159,11 @@ rpa.gov.uk: agreement_signed: true mcga.gov.uk: owner: Maritime and Coastguard Agency - agreement_signed: true + agreement_signed: true +metoffice.gov.uk: + owner: Met Office + agreement_signed: false + crown: true # Local Government aberdeencityandshire-sdpa.gov.uk: diff --git a/app/main/views/agreement.py b/app/main/views/agreement.py index 9985bd032..c109ff3de 100644 --- a/app/main/views/agreement.py +++ b/app/main/views/agreement.py @@ -2,12 +2,11 @@ from flask import render_template, send_file from flask_login import login_required from app.main import main -from app.main.views.sub_navigation_dictionaries import features_nav from app.main.s3_client import get_mou +from app.main.views.sub_navigation_dictionaries import features_nav from app.utils import AgreementInfo - @main.route('/agreement') @login_required def agreement(): diff --git a/app/utils.py b/app/utils.py index 8f5719529..3aeb6f373 100644 --- a/app/utils.py +++ b/app/utils.py @@ -484,7 +484,7 @@ class AgreementInfo: 'agreement.'.format(self.owner) ) - if self.crown_status is not None: + if self.crown_status is False: return (( '{} Download a copy.' ).format(self._acceptance_required, download_link)) @@ -504,7 +504,7 @@ class AgreementInfo: @property def crown_status_or_404(self): - if self.crown_status is None: + if self.crown_status in {None, True}: abort(404) return self.crown_status diff --git a/tests/app/main/views/test_agreement.py b/tests/app/main/views/test_agreement.py index 1376e8770..532de16da 100644 --- a/tests/app/main/views/test_agreement.py +++ b/tests/app/main/views/test_agreement.py @@ -15,7 +15,7 @@ class _MockS3Object(): @pytest.mark.parametrize('email_address, expected_status', [ - ('test@cabinet-office.gov.uk', 200), + ('test@cabinet-office.gov.uk', 404), ('test@aylesburytowncouncil.gov.uk', 200), ('test@unknown.gov.uk', 404), ]) @@ -36,11 +36,11 @@ def test_show_agreement_page( @pytest.mark.parametrize('email_address, expected_file_fetched, expected_file_served', [ - ( + pytest.mark.xfail(( 'test@cabinet-office.gov.uk', 'crown.pdf', 'GOV.UK Notify data sharing and financial agreement.pdf', - ), + ), raises=AssertionError), ( 'test@aylesburytowncouncil.gov.uk', 'non-crown.pdf', diff --git a/tests/app/main/views/test_index.py b/tests/app/main/views/test_index.py index c4d64a524..f87b43951 100644 --- a/tests/app/main/views/test_index.py +++ b/tests/app/main/views/test_index.py @@ -155,6 +155,23 @@ def test_terms_is_generic_if_user_is_not_logged_in( 'we already have one in place with your organisation.' ), ), + ( + 'michael.fish@metoffice.gov.uk', + ( + 'Your organisation (Met Office) must also accept our data ' + 'sharing and financial agreement. Contact us to get a copy.' + ), + partial( + url_for, + 'main.feedback', + ticket_type='ask-question-give-feedback', + body='agreement-with-owner', + ), + ( + 'Contact us to get a copy of the agreement (Met Office ' + 'hasn’t accepted it yet).' + ), + ), ]) def test_terms_tells_logged_in_users_what_we_know_about_their_agreement( mocker,