From a256b9c33a60bb357ff2f4c437697208fc20a324 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 12 Jul 2019 09:03:35 +0100 Subject: [PATCH] Remove the user-specific agreement pages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We used to give users the right version of the agreement by guessing their organisation from their email address. Now we do it by looking at the organisation of the service they’re looking at. In other words, users should only be downloading the agreement as part of the go live journey, not outside it. This is because we think that users will get confused if they download the agreement and: - find there’s nowhere to physically sign it - think that accepting the agreement is all they need to do to go live Maintaining two paths to download the agreement also makes the code more complicated, and makes it harder to update the content on these pages. --- app/main/views/agreement.py | 30 +--- app/main/views/service_settings.py | 4 +- app/models/organisation.py | 50 +----- app/navigation.py | 8 - .../views/agreement/_agreement-signed.html | 2 +- app/templates/views/agreement/agreement.html | 25 --- app/templates/views/terms-of-use.html | 9 -- tests/app/main/views/test_agreement.py | 152 ++++++------------ tests/app/main/views/test_index.py | 102 +----------- 9 files changed, 64 insertions(+), 318 deletions(-) delete mode 100644 app/templates/views/agreement/agreement.html diff --git a/app/main/views/agreement.py b/app/main/views/agreement.py index c906f6339..ce0851d30 100644 --- a/app/main/views/agreement.py +++ b/app/main/views/agreement.py @@ -6,28 +6,18 @@ from flask_login import current_user from app import current_service from app.main import main from app.main.forms import AcceptAgreementForm -from app.main.views.sub_navigation_dictionaries import features_nav from app.s3_client.s3_mou_client import get_mou -from app.utils import user_has_permissions, user_is_logged_in - - -@main.route('/agreement') -@user_is_logged_in -def agreement(): - return render_template( - 'views/agreement/{}.html'.format(current_user.default_organisation.as_jinja_template), - owner=current_user.default_organisation.name, - navigation_links=features_nav(), - ) +from app.utils import user_has_permissions @main.route('/services//agreement') @user_has_permissions('manage_service') def service_agreement(service_id): - return render_template( - 'views/agreement/service-{}.html'.format(current_service.organisation.as_jinja_template), - owner=current_service.organisation.name, - ) + if current_service.organisation.crown is None: + return render_template('views/agreement/service-agreement-choose.html') + if current_service.organisation.agreement_signed: + return render_template('views/agreement/service-agreement-signed.html') + return render_template('views/agreement/service-agreement.html') @main.route('/services//agreement.pdf') @@ -82,14 +72,6 @@ def service_confirm_agreement(service_id): return render_template('views/agreement/agreement-confirm.html') -@main.route('/agreement.pdf') -@user_is_logged_in -def download_agreement(): - return send_file(**get_mou( - current_user.default_organisation.crown_status_or_404 - )) - - @main.route('/agreement/', endpoint='public_agreement') @main.route('/agreement/.pdf', endpoint='public_download_agreement') def public_agreement(variant): diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 39fc331b4..e472b1530 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -212,7 +212,9 @@ def submit_request_to_go_live(service_id): service_name=current_service.name, service_dashboard=url_for('main.service_dashboard', service_id=current_service.id, _external=True), organisation_type=str(current_service.organisation_type).title(), - agreement=current_service.organisation.as_human_readable(current_user.email_domain), + agreement=current_service.organisation.as_agreement_statement_for_go_live_request( + current_user.email_domain + ), volume_email_formatted=format_thousands(current_service.volume_email), volume_sms_formatted=format_thousands(current_service.volume_sms), volume_letter_formatted=format_thousands(current_service.volume_letter), diff --git a/app/models/organisation.py b/app/models/organisation.py index e3d971c63..5c6748f16 100644 --- a/app/models/organisation.py +++ b/app/models/organisation.py @@ -1,4 +1,4 @@ -from flask import Markup, abort +from flask import abort from werkzeug.utils import cached_property from app.models import JSONModel, ModelList @@ -71,7 +71,7 @@ class Organisation(JSONModel): self.organisation_type = None self.request_to_go_live_notes = None - def as_human_readable(self, fallback_domain): + def as_agreement_statement_for_go_live_request(self, fallback_domain): if self.agreement_signed: agreement_statement = 'Yes, on behalf of {}.'.format(self.name) elif self.name: @@ -98,52 +98,6 @@ class Organisation(JSONModel): def as_info_for_branding_request(self, fallback_domain): return self.name or 'Can’t tell (domain is {})'.format(fallback_domain) - @property - def as_jinja_template(self): - if self.crown is None: - return 'agreement-choose' - if self.agreement_signed: - return 'agreement-signed' - return 'agreement' - - def as_terms_of_use_paragraph(self, **kwargs): - return Markup(self._as_terms_of_use_paragraph(**kwargs)) - - def _as_terms_of_use_paragraph(self, terms_link, download_link, support_link, signed_in): - - if not signed_in: - return (( - '{} Sign in to download a copy ' - 'or find out if one is already in place.' - ).format(self._acceptance_required, terms_link)) - - if self.agreement_signed is None: - return (( - '{} Download the agreement or ' - 'contact us to find out if we already ' - 'have one in place with your organisation.' - ).format(self._acceptance_required, download_link, support_link)) - - if self.agreement_signed is False: - return (( - '{} Download a copy.' - ).format(self._acceptance_required, download_link)) - - return ( - 'Your organisation ({}) has already accepted the ' - 'GOV.UK Notify data sharing and financial ' - 'agreement.'.format(self.name) - ) - - @property - def _acceptance_required(self): - return ( - 'Your organisation {} must also accept our data sharing ' - 'and financial agreement.'.format( - '({})'.format(self.name) if self.name else '', - ) - ) - @property def crown_status_or_404(self): if self.crown is None: diff --git a/app/navigation.py b/app/navigation.py index c893f8210..c55bd1b89 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -122,7 +122,6 @@ class HeaderNavigation(Navigation): 'add_organisation', 'add_service', 'add_service_template', - 'agreement', 'api_callbacks', 'api_documentation', 'api_integration', @@ -162,7 +161,6 @@ class HeaderNavigation(Navigation): 'delivery_and_failure', 'delivery_status_callback', 'design_content', - 'download_agreement', 'download_notifications_csv', 'edit_data_retention', 'edit_organisation_agreement', @@ -433,7 +431,6 @@ class MainNavigation(Navigation): 'add_data_retention', 'add_organisation', 'add_service', - 'agreement', 'archive_service', 'archive_user', 'bat_phone', @@ -461,7 +458,6 @@ class MainNavigation(Navigation): 'delivery_and_failure', 'design_content', 'documentation', - 'download_agreement', 'download_notifications_csv', 'edit_data_retention', 'edit_organisation_agreement', @@ -625,7 +621,6 @@ class CaseworkNavigation(Navigation): 'add_organisation', 'add_service', 'add_service_template', - 'agreement', 'api_callbacks', 'api_documentation', 'api_integration', @@ -676,7 +671,6 @@ class CaseworkNavigation(Navigation): 'delivery_status_callback', 'design_content', 'documentation', - 'download_agreement', 'download_notifications_csv', 'edit_data_retention', 'edit_organisation_agreement', @@ -914,7 +908,6 @@ class OrgNavigation(Navigation): 'add_organisation', 'add_service', 'add_service_template', - 'agreement', 'api_callbacks', 'api_documentation', 'api_integration', @@ -959,7 +952,6 @@ class OrgNavigation(Navigation): 'delivery_status_callback', 'design_content', 'documentation', - 'download_agreement', 'download_notifications_csv', 'edit_data_retention', 'edit_provider', diff --git a/app/templates/views/agreement/_agreement-signed.html b/app/templates/views/agreement/_agreement-signed.html index 72af4a9f3..e46564575 100644 --- a/app/templates/views/agreement/_agreement-signed.html +++ b/app/templates/views/agreement/_agreement-signed.html @@ -1,5 +1,5 @@

- Your organisation ({{ owner }}) has already accepted the GOV.UK + Your organisation ({{ current_service.organisation.name }}) has already accepted the GOV.UK Notify data sharing and financial agreement. You can download a copy.

diff --git a/app/templates/views/agreement/agreement.html b/app/templates/views/agreement/agreement.html deleted file mode 100644 index a5fbfcacd..000000000 --- a/app/templates/views/agreement/agreement.html +++ /dev/null @@ -1,25 +0,0 @@ -{% 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 -

- - {% include 'views/agreement/_agreement.html'%} - -
-
- -{% endblock %} diff --git a/app/templates/views/terms-of-use.html b/app/templates/views/terms-of-use.html index 0a63b0b7a..c53a0c6e2 100644 --- a/app/templates/views/terms-of-use.html +++ b/app/templates/views/terms-of-use.html @@ -13,15 +13,6 @@ These terms apply to your service’s use of GOV.UK Notify. You must be the service manager to accept them.

-

- {{ current_user.default_organisation.as_terms_of_use_paragraph( - terms_link=url_for('main.sign_in', next=url_for('main.terms')), - download_link=url_for('.agreement'), - support_link=url_for('.feedback', ticket_type='ask-question-give-feedback', body='agreement'), - signed_in=current_user.is_authenticated - )}} -

-

When using Notify

You must:

    diff --git a/tests/app/main/views/test_agreement.py b/tests/app/main/views/test_agreement.py index 2dbf29274..9d7193ad9 100644 --- a/tests/app/main/views/test_agreement.py +++ b/tests/app/main/views/test_agreement.py @@ -9,7 +9,6 @@ from freezegun import freeze_time from tests import organisation_json from tests.conftest import ( SERVICE_ONE_ID, - mock_get_organisation_by_domain, mock_get_service_organisation, normalize_spaces, ) @@ -24,49 +23,65 @@ class _MockS3Object(): return {'Body': BytesIO(self.data)} -@pytest.mark.parametrize('endpoint, extra_args, organisation_mock, link_selector, expected_back_links', [ - ( - 'main.agreement', - {}, - mock_get_organisation_by_domain, - 'main .column-two-thirds a', - [] - ), - ( - 'main.service_agreement', - {'service_id': SERVICE_ONE_ID}, - mock_get_service_organisation, - 'main .column-five-sixths a', - [ - partial(url_for, 'main.request_to_go_live', service_id=SERVICE_ONE_ID) - ] - ), -]) @pytest.mark.parametrize('agreement_signed, crown, expected_links', [ ( True, True, [ - partial(url_for, 'main.service_download_agreement', service_id=SERVICE_ONE_ID), + ( + ['govuk-back-link'], + partial(url_for, 'main.request_to_go_live', service_id=SERVICE_ONE_ID), + ), + ( + [], + partial(url_for, 'main.service_download_agreement', service_id=SERVICE_ONE_ID), + ), ] ), ( False, False, [ - partial(url_for, 'main.service_download_agreement', service_id=SERVICE_ONE_ID), - partial(url_for, 'main.service_accept_agreement', service_id=SERVICE_ONE_ID), + ( + ['govuk-back-link'], + partial(url_for, 'main.request_to_go_live', service_id=SERVICE_ONE_ID), + ), + ( + [], + partial(url_for, 'main.service_download_agreement', service_id=SERVICE_ONE_ID), + ), + ( + ['button'], + partial(url_for, 'main.service_accept_agreement', service_id=SERVICE_ONE_ID), + ), ] ), ( False, True, [ - partial(url_for, 'main.service_download_agreement', service_id=SERVICE_ONE_ID), - partial(url_for, 'main.service_accept_agreement', service_id=SERVICE_ONE_ID), + ( + ['govuk-back-link'], + partial(url_for, 'main.request_to_go_live', service_id=SERVICE_ONE_ID), + ), + ( + [], + partial(url_for, 'main.service_download_agreement', service_id=SERVICE_ONE_ID), + ), + ( + ['button'], + partial(url_for, 'main.service_accept_agreement', service_id=SERVICE_ONE_ID), + ), ] ), ( None, None, [ - partial(url_for, 'main.support'), + ( + ['govuk-back-link'], + partial(url_for, 'main.request_to_go_live', service_id=SERVICE_ONE_ID), + ), + ( + [], + partial(url_for, 'main.support'), + ), ] ), ]) @@ -75,27 +90,22 @@ def test_show_agreement_page( mocker, fake_uuid, mock_has_jobs, - mock_get_service_organisation, agreement_signed, crown, expected_links, - endpoint, - extra_args, - organisation_mock, - link_selector, - expected_back_links, ): - organisation_mock( + mock_get_service_organisation( mocker, crown=crown, agreement_signed=agreement_signed, ) - expected_links = expected_back_links + expected_links - page = client_request.get(endpoint, **extra_args) - links = page.select(link_selector) + page = client_request.get('main.service_agreement', service_id=SERVICE_ONE_ID) + links = page.select('main .column-five-sixths a') assert len(links) == len(expected_links) for index, link in enumerate(links): - assert link['href'] == expected_links[index]() + classes, url = expected_links[index] + assert link.get('class', []) == classes + assert link['href'] == url() @pytest.mark.parametrize('crown, expected_status, expected_file_fetched, expected_file_served', ( @@ -448,76 +458,6 @@ def test_confirm_agreement_page_persists( ) -@pytest.mark.parametrize('crown, expected_file_fetched, expected_file_served', [ - ( - True, - 'crown.pdf', - 'GOV.UK Notify data sharing and financial agreement.pdf', - ), - ( - False, - 'non-crown.pdf', - 'GOV.UK Notify data sharing and financial agreement (non-crown).pdf', - ), -]) -def test_downloading_agreement( - logged_in_client, - mocker, - fake_uuid, - crown, - expected_file_fetched, - expected_file_served, -): - mock_get_s3_object = mocker.patch( - 'app.s3_client.s3_mou_client.get_s3_object', - return_value=_MockS3Object(b'foo') - ) - mock_get_organisation_by_domain( - mocker, - crown=crown, - ) - 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.s3_client.s3_mou_client.get_s3_object', - return_value=_MockS3Object() - ) - mock_get_organisation_by_domain( - mocker, - crown=None, - ) - 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.s3_client.s3_mou_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 == [] - - @pytest.mark.parametrize('endpoint', ( 'main.public_agreement', 'main.public_download_agreement', diff --git a/tests/app/main/views/test_index.py b/tests/app/main/views/test_index.py index b49204057..473e4bf0a 100644 --- a/tests/app/main/views/test_index.py +++ b/tests/app/main/views/test_index.py @@ -1,15 +1,9 @@ -from functools import partial - import pytest from bs4 import BeautifulSoup from flask import url_for from app.main.forms import FieldWithNoneOption -from tests.conftest import ( - mock_get_organisation_by_domain, - normalize_spaces, - sample_uuid, -) +from tests.conftest import normalize_spaces, sample_uuid def test_non_logged_in_user_can_see_homepage( @@ -144,96 +138,12 @@ def test_old_integration_testing_page( ) -def test_terms_is_generic_if_user_is_not_logged_in( - client -): - response = client.get(url_for('main.terms')) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - - assert normalize_spaces(page.select('main p')[1].text) == ( - 'Your organisation must also accept our data sharing and ' - 'financial agreement. Sign in to download a copy or find out ' - 'if one is already in place.' - ) - - -@pytest.mark.parametrize(( - 'name,' - 'agreement_signed,' - 'expected_terms_paragraph,' - 'expected_terms_link,' -), [ - ( - 'Cabinet Office', - True, - ( - 'Your organisation (Cabinet Office) has already accepted ' - 'the GOV.UK Notify data sharing and financial agreement.' - ), - None, - ), - ( - 'Aylesbury Town Council', - False, - ( - 'Your organisation (Aylesbury Town Council) must also ' - 'accept our data sharing and financial agreement. Download ' - 'a copy.' - ), - partial( - url_for, - 'main.agreement', - ), - ), - ( - None, - None, - ( - 'Your organisation must also accept our data sharing and ' - 'financial agreement. Download the agreement or contact us ' - 'to find out if we already have one in place with your ' - 'organisation.' - ), - partial( - url_for, - 'main.agreement', - ), - ), - ( - 'Met Office', - False, - ( - 'Your organisation (Met Office) must also accept our data ' - 'sharing and financial agreement. Download a copy.' - ), - partial( - url_for, - 'main.agreement', - ), - ), -]) -def test_terms_tells_logged_in_users_what_we_know_about_their_agreement( - mocker, - fake_uuid, - client_request, - name, - agreement_signed, - expected_terms_paragraph, - expected_terms_link, -): - mock_get_organisation_by_domain( - mocker, - name=name, - agreement_signed=agreement_signed, - ) +def test_terms_page_has_correct_content(client_request): terms_page = client_request.get('main.terms') - - 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(terms_page.select('main p')[0].text) == ( + 'These terms apply to your service’s use of GOV.UK Notify. ' + 'You must be the service manager to accept them.' + ) def test_css_is_served_from_correct_path(client_request):