Remove the user-specific agreement pages

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

@@ -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
+from app.utils import user_has_permissions
-
-
-@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(),
-    )
 
 
 @main.route('/services/<uuid:service_id>/agreement')
 @user_has_permissions('manage_service')
 def service_agreement(service_id):
-    return render_template(
+    if current_service.organisation.crown is None:
-        'views/agreement/service-{}.html'.format(current_service.organisation.as_jinja_template),
+        return render_template('views/agreement/service-agreement-choose.html')
-        owner=current_service.organisation.name,
+    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/<uuid:service_id>/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/<variant>', endpoint='public_agreement')
 @main.route('/agreement/<variant>.pdf', endpoint='public_download_agreement')
 def public_agreement(variant):

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

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 ((
-                '{} <a href="{}">Sign in</a> 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 ((
-                '{} <a href="{}">Download the agreement</a> or '
-                '<a href="{}">contact us</a> 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 ((
-                '{} <a href="{}">Download a copy</a>.'
-            ).format(self._acceptance_required, download_link))
-
-        return (
-            'Your organisation ({}) has already accepted the '
-            'GOV.UK&nbsp;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:

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',

app/templates/views/agreement/_agreement-signed.html

@@ -1,5 +1,5 @@
 <p>
-  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
   <a href="{{ url_for('main.service_download_agreement', service_id=current_service.id) }}">download a copy</a>.
 </p>

app/templates/views/agreement/agreement.html

@@ -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 %}
-
-<div class="grid-row">
-  <div class="column-one-third">
-    {{ sub_navigation(navigation_links) }}
-  </div>
-  <div class="column-two-thirds">
-
-    <h1 class="heading-large">
-      GOV.UK Notify data sharing and financial agreement
-    </h1>
-
-    {% include 'views/agreement/_agreement.html'%}
-
-  </div>
-</div>
-
-{% endblock %}

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.
   </p>
 
-  <p>
-    {{ 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
-    )}}
-  </p>
-
   <h2 class="heading-medium">When using Notify</h2>
   <p>You must:</p>
   <ul class="list list-bullet">

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('main.service_agreement', service_id=SERVICE_ONE_ID)
-    page = client_request.get(endpoint, **extra_args)
+    links = page.select('main .column-five-sixths a')
-    links = page.select(link_selector)
     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',

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 (
+from tests.conftest import normalize_spaces, sample_uuid
-    mock_get_organisation_by_domain,
-    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(
+def test_terms_page_has_correct_content(client_request):
-    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,
-    )
     terms_page = client_request.get('main.terms')
-
+    assert normalize_spaces(terms_page.select('main p')[0].text) == (
-    assert normalize_spaces(terms_page.select('main p')[1].text) == expected_terms_paragraph
+        'These terms apply to your service’s use of GOV.UK Notify. '
-    if expected_terms_link:
+        'You must be the service manager to accept them.'
-        assert terms_page.select_one('main p a')['href'] == expected_terms_link()
+    )
-    else:
-        assert not terms_page.select_one('main p').select('a')
 
 
 def test_css_is_served_from_correct_path(client_request):