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):