From 13d05c5461f1637c5c30f15b405a5f67c3c10802 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Fri, 13 Jul 2018 16:47:26 +0100
Subject: [PATCH 01/10] Allow a platform admin user to set flexible data
 retention for a service for a specified notification type. Still a work in
 progress

---
 app/main/views/service_settings.py            | 14 ++++++++++
 app/notify_client/service_api_client.py       | 20 +++++++++++++
 app/templates/views/service-settings.html     |  5 ++++
 .../service-settings/set-data-retention.html  | 28 +++++++++++++++++++
 4 files changed, 67 insertions(+)
 create mode 100644 app/templates/views/service-settings/set-data-retention.html

diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py
index efe41b6dc..525bb1857 100644
--- a/app/main/views/service_settings.py
+++ b/app/main/views/service_settings.py
@@ -86,6 +86,7 @@ def service_settings(service_id):
     )
 
     free_sms_fragment_limit = billing_api_client.get_free_sms_fragment_limit_for_year(service_id)
+    data_retention = service_api_client.get_service_data_retention(service_id)
 
     return render_template(
         'views/service-settings.html',
@@ -104,6 +105,7 @@ def service_settings(service_id):
         free_sms_fragment_limit=free_sms_fragment_limit,
         prefix_sms=current_service['prefix_sms'],
         organisation=organisation,
+        data_retention=data_retention,
     )
 
 
@@ -1008,6 +1010,18 @@ def branding_request(service_id):
     )
 
 
+@main.route("/services/<service_id>/set-data-retention", methods=['GET', 'POST'])
+@login_required
+@user_is_platform_admin
+def set_data_retention(service_id):
+    results = service_api_client.get_service_data_retention(service_id)
+    print("DATA RETENTION: ", results[0])
+    return render_template(
+        'views/service-settings/set-data-retention.html',
+        data_retentions=results
+    )
+
+
 def get_branding_as_value_and_label(email_branding):
     return [
         (branding['id'], branding['name'])
diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py
index a60fb135d..b2d2f79b5 100644
--- a/app/notify_client/service_api_client.py
+++ b/app/notify_client/service_api_client.py
@@ -480,3 +480,23 @@ class ServiceAPIClient(NotifyAdminAPIClient):
             "updated_by_id": user_id
         }
         return self.post("/service/{}/delivery-receipt-api".format(service_id), data)
+
+    def create_service_data_retention(self, service_id, notification_type, days_of_retention):
+        data = {
+            "notification_type": notification_type,
+            "days_of_retention": days_of_retention
+        }
+
+        return self.post("/service/{}/data-retention".format(service_id), data)
+
+    def update_service_data_retention(self, service_id, data_retention_id, days_of_retention):
+        data = {
+            "days_of_retention": days_of_retention
+        }
+        return self.post("/service/{}/data-retention/{}".format(service_id, data_retention_id), data)
+
+    def get_service_data_retention(self, service_id):
+        return self.get("/service/{}/data-retention".format(service_id))
+
+    def get_service_data_retention_by_id(self, service_id, data_retention_id):
+        return self.get("service/{}/data-retention/{}".format(service_id, data_retention_id))
diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html
index 8aa628d4d..77d7799e9 100644
--- a/app/templates/views/service-settings.html
+++ b/app/templates/views/service-settings.html
@@ -320,6 +320,11 @@
           {{ text_field(letter_branding) }}
           {{ edit_field('Change', url_for('.set_letter_branding', service_id=current_service.id)) }}
         {% endcall %}
+        {% call row() %}
+          {{ text_field('Data Retention')}}
+          {{ text_field(data_retention[0]['notification_type']) }}
+          {{ edit_field('Change', url_for('.set_data_retention', service_id=current_service.id)) }}
+        {% endcall %}
       {% endcall %}
 
       <ul>
diff --git a/app/templates/views/service-settings/set-data-retention.html b/app/templates/views/service-settings/set-data-retention.html
new file mode 100644
index 000000000..42c318b67
--- /dev/null
+++ b/app/templates/views/service-settings/set-data-retention.html
@@ -0,0 +1,28 @@
+{% extends "withnav_template.html" %}
+{% from "components/textbox.html" import textbox %}
+{% from "components/page-footer.html" import page_footer %}
+
+{% block service_page_title %}
+  Data retention
+{% endblock %}
+
+{% block maincolumn_content %}
+
+  <div class="grid-row">
+    <div class="column-five-sixths">
+      <h1 class="heading-large">Emails</h1>
+        <p>
+          Add new data retention here
+        </p>
+         {% for item in data_retentions %}
+            {{ item.notification_type }}
+            {{ item.days_of_retention }}
+        {% endfor %}
+          {{ page_footer(
+            back_link=url_for('.service_settings', service_id=current_service.id),
+            back_link_text='Back to settings'
+          ) }}
+    </div>
+  </div>
+
+{% endblock %}
\ No newline at end of file

From 4b8b571a8768ab71cebb95b0338bec957b016fa2 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Tue, 17 Jul 2018 14:39:04 +0100
Subject: [PATCH 02/10] Adding views to view, add and edit service data
 retention policies. Only visible to a platform admin. A service can have a
 custom number of days to retain the notification data for each notification
 type.

---
 app/__init__.py                               |  9 ++++
 app/main/forms.py                             | 24 ++++++++++
 app/main/views/service_settings.py            | 44 ++++++++++++++++---
 app/navigation.py                             | 12 +++++
 app/templates/views/service-settings.html     |  6 ++-
 .../service-settings/data-retention.html      | 44 +++++++++++++++++++
 .../service-settings/data-retention/add.html  | 25 +++++++++++
 .../service-settings/data-retention/edit.html | 25 +++++++++++
 .../service-settings/set-data-retention.html  | 28 ------------
 .../test_service_setting_permissions.py       |  2 +
 tests/app/main/views/test_service_settings.py |  3 ++
 tests/conftest.py                             |  6 +++
 12 files changed, 193 insertions(+), 35 deletions(-)
 create mode 100644 app/templates/views/service-settings/data-retention.html
 create mode 100644 app/templates/views/service-settings/data-retention/add.html
 create mode 100644 app/templates/views/service-settings/data-retention/edit.html
 delete mode 100644 app/templates/views/service-settings/set-data-retention.html

diff --git a/app/__init__.py b/app/__init__.py
index bff65c3b1..24ff7a6d0 100644
--- a/app/__init__.py
+++ b/app/__init__.py
@@ -338,6 +338,14 @@ def valid_phone_number(phone_number):
         return False
 
 
+def format_notification_type(notification_type):
+    return {
+        'email': 'Email',
+        'sms': 'SMS',
+        'letter': 'Letter'
+    }[notification_type]
+
+
 def format_notification_status(status, template_type):
     return {
         'email': {
@@ -640,6 +648,7 @@ def add_template_filters(application):
         format_datetime_relative,
         format_delta,
         format_notification_status,
+        format_notification_type,
         format_notification_status_as_time,
         format_notification_status_as_field_status,
         format_notification_status_as_url,
diff --git a/app/main/forms.py b/app/main/forms.py
index 826080376..a22a72823 100644
--- a/app/main/forms.py
+++ b/app/main/forms.py
@@ -1024,3 +1024,27 @@ class BrandingOptionsEmail(StripWhitespaceForm):
             DataRequired()
         ],
     )
+
+
+class ServiceDataRetentionForm(StripWhitespaceForm):
+
+    notification_type = RadioField(
+        'What notification type?',
+        choices=[
+            ('email', 'Email'),
+            ('sms', 'SMS'),
+            ('letter', 'Letter'),
+        ],
+        validators=[DataRequired()],
+    )
+    days_of_retention = IntegerField(label="Days of retention",
+                                     validators=[validators.NumberRange(min=3, max=90,
+                                                                        message="Must be between 3 and 90")],
+                                     )
+
+
+class ServiceDataRetentionEditForm(StripWhitespaceForm):
+    days_of_retention = IntegerField(label="Days of retention",
+                                     validators=[validators.NumberRange(min=3, max=90,
+                                                                        message="Must be between 3 and 90")],
+                                     )
diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py
index 525bb1857..782c7f3a0 100644
--- a/app/main/views/service_settings.py
+++ b/app/main/views/service_settings.py
@@ -36,6 +36,8 @@ from app.main.forms import (
     RequestToGoLiveForm,
     ServiceBasicViewForm,
     ServiceContactLinkForm,
+    ServiceDataRetentionEditForm,
+    ServiceDataRetentionForm,
     ServiceEditInboundNumberForm,
     ServiceInboundNumberForm,
     ServiceLetterContactBlockForm,
@@ -1010,15 +1012,47 @@ def branding_request(service_id):
     )
 
 
-@main.route("/services/<service_id>/set-data-retention", methods=['GET', 'POST'])
+@main.route("/services/<service_id>/data-retention", methods=['GET'])
 @login_required
 @user_is_platform_admin
-def set_data_retention(service_id):
+def data_retention(service_id):
     results = service_api_client.get_service_data_retention(service_id)
-    print("DATA RETENTION: ", results[0])
+    return render_template('views/service-settings/data-retention.html',
+                           data_retention_settings=results)
+
+
+@main.route("/services/<service_id>/data-retention/add", methods=['GET', 'POST'])
+@login_required
+@user_is_platform_admin
+def add_data_retention(service_id):
+    form = ServiceDataRetentionForm()
+    if form.validate_on_submit():
+        service_api_client.create_service_data_retention(service_id,
+                                                         form.notification_type.data,
+                                                         form.days_of_retention.data)
+        return redirect(url_for('.data_retention', service_id=service_id))
     return render_template(
-        'views/service-settings/set-data-retention.html',
-        data_retentions=results
+        'views/service-settings/data-retention/add.html',
+        form=form
+    )
+
+
+@main.route("/services/<service_id>/data-retention/<data_retention_id>/edit", methods=['GET', 'POST'])
+@login_required
+@user_is_platform_admin
+def edit_data_retention(service_id, data_retention_id):
+    form = ServiceDataRetentionEditForm()
+    if request.method == 'GET':
+        data_retention_item = service_api_client.get_service_data_retention_by_id(service_id, data_retention_id)
+    if form.validate_on_submit():
+        service_api_client.update_service_data_retention(service_id, data_retention_id, form.days_of_retention.data)
+        return redirect(url_for('.data_retention', service_id=service_id))
+    return render_template(
+        'views/service-settings/data-retention/edit.html',
+        form=form,
+        data_retention_id=data_retention_id,
+        notification_type=data_retention_item['notification_type'],
+        days_of_retention=data_retention_item['days_of_retention']
     )
 
 
diff --git a/app/navigation.py b/app/navigation.py
index 4860e4adf..fef43debe 100644
--- a/app/navigation.py
+++ b/app/navigation.py
@@ -101,6 +101,7 @@ class HeaderNavigation(Navigation):
         'accept_invite',
         'accept_org_invite',
         'action_blocked',
+        'add_data_retention',
         'add_service',
         'add_service_template',
         'add_template_by_type',
@@ -131,12 +132,14 @@ class HeaderNavigation(Navigation):
         'conversation_updates',
         'cookies',
         'create_api_key',
+        'data_retention',
         'delete_service_template',
         'delivery_and_failure',
         'delivery_status_callback',
         'design_content',
         'download_agreement',
         'download_notifications_csv',
+        'edit_data_retention',
         'edit_organisation_name',
         'edit_provider',
         'edit_service_template',
@@ -366,6 +369,7 @@ class MainNavigation(Navigation):
     exclude = {
         'accept_invite',
         'accept_org_invite',
+        'add_data_retention',
         'add_organisation',
         'add_service',
         'agreement',
@@ -385,11 +389,13 @@ class MainNavigation(Navigation):
         'conversation_updates',
         'cookies',
         'create_email_branding',
+        'data_retention',
         'delivery_and_failure',
         'design_content',
         'documentation',
         'download_agreement',
         'download_notifications_csv',
+        'edit_data_retention',
         'edit_organisation_name',
         'edit_provider',
         'edit_user_org_permissions',
@@ -517,6 +523,7 @@ class CaseworkNavigation(Navigation):
         'accept_invite',
         'accept_org_invite',
         'action_blocked',
+        'add_data_retention',
         'add_organisation',
         'add_service',
         'add_service_template',
@@ -549,6 +556,7 @@ class CaseworkNavigation(Navigation):
         'cookies',
         'create_api_key',
         'create_email_branding',
+        'data_retention',
         'delete_service_template',
         'delivery_and_failure',
         'delivery_status_callback',
@@ -556,6 +564,7 @@ class CaseworkNavigation(Navigation):
         'documentation',
         'download_agreement',
         'download_notifications_csv',
+        'edit_data_retention',
         'edit_organisation_name',
         'edit_provider',
         'edit_service_template',
@@ -741,6 +750,7 @@ class OrgNavigation(Navigation):
         'accept_invite',
         'accept_org_invite',
         'action_blocked',
+        'add_data_retention',
         'add_organisation',
         'add_service',
         'add_service_template',
@@ -773,6 +783,7 @@ class OrgNavigation(Navigation):
         'cookies',
         'create_api_key',
         'create_email_branding',
+        'data_retention',
         'delete_service_template',
         'delivery_and_failure',
         'delivery_status_callback',
@@ -780,6 +791,7 @@ class OrgNavigation(Navigation):
         'documentation',
         'download_agreement',
         'download_notifications_csv',
+        'edit_data_retention',
         'edit_provider',
         'edit_service_template',
         'edit_user_permissions',
diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html
index 77d7799e9..2297fbfe2 100644
--- a/app/templates/views/service-settings.html
+++ b/app/templates/views/service-settings.html
@@ -322,8 +322,10 @@
         {% endcall %}
         {% call row() %}
           {{ text_field('Data Retention')}}
-          {{ text_field(data_retention[0]['notification_type']) }}
-          {{ edit_field('Change', url_for('.set_data_retention', service_id=current_service.id)) }}
+          {% call field() %}
+              {{ data_retention | join(', ', attribute='notification_type') }}
+          {% endcall %}
+          {{ edit_field('Change', url_for('.data_retention', service_id=current_service.id)) }}
         {% endcall %}
       {% endcall %}
 
diff --git a/app/templates/views/service-settings/data-retention.html b/app/templates/views/service-settings/data-retention.html
new file mode 100644
index 000000000..67322dffc
--- /dev/null
+++ b/app/templates/views/service-settings/data-retention.html
@@ -0,0 +1,44 @@
+{% extends "withnav_template.html" %}
+{% from "components/api-key.html" import api_key %}
+{% from "components/page-footer.html" import page_footer %}
+{% from "components/table.html" import row_group, row, text_field, edit_field, field, boolean_field, list_table with context %}
+
+{% block service_page_title %}
+  Email reply to addresses
+{% endblock %}
+
+{% block maincolumn_content %}
+
+  <div class="grid-row bottom-gutter">
+    <div class="column-two-thirds">
+      <h1 class="heading-large">
+        Data retention
+      </h1>
+    </div>
+   <div class="column-one-third">
+	    <a href="{{ url_for('.add_data_retention', service_id=current_service.id) }}" class="button align-with-heading">Add data retention</a>
+  </div>
+  </div>
+  <div class="grid-row bottom-gutter">
+      <div class="column-full">
+          By default data is kept for 7 days
+      </div>
+  </div>
+  <div class="user-list">
+    {% if not data_retention_settings %}
+      <div class="user-list-item">
+        <span class="hint">You haven’t added any data retention</span>
+      </div>
+    {% endif %}
+    {% for item in data_retention_settings %}
+      <div class="user-list-item">
+
+          <p>
+            {{ item.notification_type | format_notification_type }} notifications will be kept for {{ item.days_of_retention }} days
+              <a class="user-list-edit-link" href="{{ url_for('.edit_data_retention', service_id =current_service.id, data_retention_id = item.id) }}">Change</a>
+          </p>
+      </div>
+    {% endfor %}
+  </div>
+
+{% endblock %}
diff --git a/app/templates/views/service-settings/data-retention/add.html b/app/templates/views/service-settings/data-retention/add.html
new file mode 100644
index 000000000..da70d086f
--- /dev/null
+++ b/app/templates/views/service-settings/data-retention/add.html
@@ -0,0 +1,25 @@
+{% extends "withnav_template.html" %}
+{% from "components/textbox.html" import textbox %}
+{% from "components/page-footer.html" import page_footer %}
+
+{% from "components/radios.html" import radios %}
+
+{% block service_page_title %}
+  Data retention
+{% endblock %}
+
+{% block maincolumn_content %}
+
+
+  <h1 class="heading-large">Set data retention</h1>
+    <form method="post">
+      {{ radios(form.notification_type) }}
+      {{  textbox(form.days_of_retention) }}
+      {{ page_footer(
+      'Add',
+      back_link=url_for('.add_data_retention', service_id=current_service.id),
+      back_link_text='Back'
+    ) }}
+    </form>
+
+{% endblock %}
\ No newline at end of file
diff --git a/app/templates/views/service-settings/data-retention/edit.html b/app/templates/views/service-settings/data-retention/edit.html
new file mode 100644
index 000000000..21a7c8736
--- /dev/null
+++ b/app/templates/views/service-settings/data-retention/edit.html
@@ -0,0 +1,25 @@
+{% extends "withnav_template.html" %}
+{% from "components/textbox.html" import textbox %}
+{% from "components/page-footer.html" import page_footer %}
+
+{% from "components/radios.html" import radios %}
+
+{% block service_page_title %}
+  Data retention
+{% endblock %}
+
+{% block maincolumn_content %}
+
+
+  <h1 class="heading-large">Set data retention</h1>
+    <form method="post">
+      {{ notification_type | capitalize}}
+      {{  textbox(form.days_of_retention) }}
+      {{ page_footer(
+      'Save',
+      back_link=url_for('.edit_data_retention', service_id=current_service.id, data_retention_id=data_retention_id),
+      back_link_text='Back'
+    ) }}
+    </form>
+
+{% endblock %}
\ No newline at end of file
diff --git a/app/templates/views/service-settings/set-data-retention.html b/app/templates/views/service-settings/set-data-retention.html
deleted file mode 100644
index 42c318b67..000000000
--- a/app/templates/views/service-settings/set-data-retention.html
+++ /dev/null
@@ -1,28 +0,0 @@
-{% extends "withnav_template.html" %}
-{% from "components/textbox.html" import textbox %}
-{% from "components/page-footer.html" import page_footer %}
-
-{% block service_page_title %}
-  Data retention
-{% endblock %}
-
-{% block maincolumn_content %}
-
-  <div class="grid-row">
-    <div class="column-five-sixths">
-      <h1 class="heading-large">Emails</h1>
-        <p>
-          Add new data retention here
-        </p>
-         {% for item in data_retentions %}
-            {{ item.notification_type }}
-            {{ item.days_of_retention }}
-        {% endfor %}
-          {{ page_footer(
-            back_link=url_for('.service_settings', service_id=current_service.id),
-            back_link_text='Back to settings'
-          ) }}
-    </div>
-  </div>
-
-{% endblock %}
\ No newline at end of file
diff --git a/tests/app/main/views/service_settings/test_service_setting_permissions.py b/tests/app/main/views/service_settings/test_service_setting_permissions.py
index 40b3b85a7..51cd9d805 100644
--- a/tests/app/main/views/service_settings/test_service_setting_permissions.py
+++ b/tests/app/main/views/service_settings/test_service_setting_permissions.py
@@ -16,6 +16,7 @@ def get_service_settings_page(
     no_reply_to_email_addresses,
     no_letter_contact_blocks,
     single_sms_sender,
+    mock_get_service_data_retention,
 ):
     client_request.login(platform_admin_user)
     return functools.partial(client_request.get, 'main.service_settings', service_id=service_one['id'])
@@ -103,6 +104,7 @@ def test_normal_user_doesnt_see_any_toggle_buttons(
     mock_get_letter_email_branding,
     mock_get_inbound_number_for_service,
     mock_get_free_sms_fragment_limit,
+    mock_get_service_data_retention
 ):
     page = client_request.get('main.service_settings', service_id=service_one['id'])
     toggles = page.find('a', {'class': 'button'})
diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py
index dcb37e915..3d0c6603c 100644
--- a/tests/app/main/views/test_service_settings.py
+++ b/tests/app/main/views/test_service_settings.py
@@ -40,6 +40,7 @@ def mock_get_service_settings_page_common(
     mock_get_letter_email_branding,
     mock_get_inbound_number_for_service,
     mock_get_free_sms_fragment_limit,
+    mock_get_service_data_retention,
 ):
     return
 
@@ -94,6 +95,7 @@ def mock_get_service_settings_page_common(
         'Free text message allowance 250,000 Change',
         'Email branding GOV.UK Change',
         'Letter branding HM Government Change',
+        'Data Retention Change'
 
     ]),
 ])
@@ -2527,6 +2529,7 @@ def test_service_settings_when_inbound_number_is_not_set(
     mocker,
     mock_get_letter_email_branding,
     mock_get_free_sms_fragment_limit,
+    mock_get_service_data_retention,
 ):
     mocker.patch('app.inbound_number_client.get_inbound_sms_number_for_service',
                  return_value={'data': {}})
diff --git a/tests/conftest.py b/tests/conftest.py
index f9cb681c5..d02db1a6b 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2670,6 +2670,12 @@ def normalize_spaces(input):
     return normalize_spaces(' '.join(item.text for item in input))
 
 
+@pytest.fixture(scope='function')
+def mock_get_service_data_retention(mocker):
+    return mocker.patch('app.service_api_client.get_service_data_retention',
+                        return_value={})
+
+
 @pytest.fixture(scope='function')
 def mock_get_free_sms_fragment_limit(mocker):
     sample_limit = 250000

From c2f973da95ec27e97075703cc7013ef4b42b1644 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Tue, 17 Jul 2018 15:50:40 +0100
Subject: [PATCH 03/10] Added unit test for the data retention pages.

---
 tests/app/main/views/test_service_settings.py | 59 ++++++++++++++++++-
 tests/conftest.py                             | 20 ++++++-
 2 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py
index 3d0c6603c..7c0abc223 100644
--- a/tests/app/main/views/test_service_settings.py
+++ b/tests/app/main/views/test_service_settings.py
@@ -95,7 +95,7 @@ def mock_get_service_settings_page_common(
         'Free text message allowance 250,000 Change',
         'Email branding GOV.UK Change',
         'Letter branding HM Government Change',
-        'Data Retention Change'
+        'Data Retention email Change'
 
     ]),
 ])
@@ -2793,3 +2793,60 @@ def test_submit_email_branding_request(
         'Thanks for your branding request. We’ll get back to you '
         'within one working day.'
     )
+
+
+def test_show_service_data_retention(
+        logged_in_platform_admin_client,
+        service_one,
+        mock_get_service_data_retention,
+
+):
+    response = logged_in_platform_admin_client.get(url_for('main.data_retention', service_id=service_one['id']))
+    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+    assert normalize_spaces(page.select_one('main p').text) == "Email notifications will be kept for 5 days Change"
+
+
+def test_view_add_service_data_retention(
+        logged_in_platform_admin_client,
+        service_one,
+
+):
+    response = logged_in_platform_admin_client.get(url_for('main.add_data_retention', service_id=service_one['id']))
+    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+    assert normalize_spaces(page.select_one('input')['value']) == "email"
+    assert page.find('input', attrs={'name': 'days_of_retention'})
+
+
+def test_add_service_data_retention(
+        logged_in_platform_admin_client,
+        service_one,
+        mock_create_service_data_retention
+):
+    response = logged_in_platform_admin_client.post(url_for('main.add_data_retention', service_id=service_one['id']),
+                                                    data={'notification_type': "email",
+                                                          'days_of_retention': 5
+                                                          }
+                                                    )
+    assert response.status_code == 302
+    settings_url = url_for(
+        'main.data_retention', service_id=service_one['id'], _external=True)
+    assert settings_url == response.location
+    assert mock_create_service_data_retention.called
+
+
+def test_update_service_data_retention(
+        logged_in_platform_admin_client,
+        service_one,
+        fake_uuid,
+        mock_update_service_data_retention,
+):
+    response = logged_in_platform_admin_client.post(url_for('main.edit_data_retention',
+                                                            service_id=service_one['id'],
+                                                            data_retention_id=fake_uuid),
+                                                    data={'days_of_retention': 5}
+                                                    )
+    assert response.status_code == 302
+    settings_url = url_for(
+        'main.data_retention', service_id=service_one['id'], _external=True)
+    assert settings_url == response.location
+    assert mock_update_service_data_retention.called
diff --git a/tests/conftest.py b/tests/conftest.py
index d02db1a6b..bf28929a1 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2672,8 +2672,26 @@ def normalize_spaces(input):
 
 @pytest.fixture(scope='function')
 def mock_get_service_data_retention(mocker):
+    data = {"id": str(fake_uuid),
+            "service_id": str(fake_uuid),
+            "service_name": "service name",
+            "notification_type": "email",
+            "days_of_retention": 5,
+            "created_at": datetime.now(),
+            "updated_at": None,
+            }
     return mocker.patch('app.service_api_client.get_service_data_retention',
-                        return_value={})
+                        return_value=[data])
+
+
+@pytest.fixture(scope='function')
+def mock_create_service_data_retention(mocker):
+    return mocker.patch('app.service_api_client.create_service_data_retention')
+
+
+@pytest.fixture(scope='function')
+def mock_update_service_data_retention(mocker):
+    return mocker.patch('app.service_api_client.update_service_data_retention')
 
 
 @pytest.fixture(scope='function')

From be51a668cf726a91f42a9e1d807914392acebac2 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Tue, 17 Jul 2018 15:56:28 +0100
Subject: [PATCH 04/10] Fix capitalisation on Data Retention

---
 app/templates/views/service-settings.html     | 2 +-
 tests/app/main/views/test_service_settings.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html
index 2297fbfe2..1f7734edb 100644
--- a/app/templates/views/service-settings.html
+++ b/app/templates/views/service-settings.html
@@ -321,7 +321,7 @@
           {{ edit_field('Change', url_for('.set_letter_branding', service_id=current_service.id)) }}
         {% endcall %}
         {% call row() %}
-          {{ text_field('Data Retention')}}
+          {{ text_field('Data retention')}}
           {% call field() %}
               {{ data_retention | join(', ', attribute='notification_type') }}
           {% endcall %}
diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py
index 7c0abc223..1b29dcca4 100644
--- a/tests/app/main/views/test_service_settings.py
+++ b/tests/app/main/views/test_service_settings.py
@@ -95,7 +95,7 @@ def mock_get_service_settings_page_common(
         'Free text message allowance 250,000 Change',
         'Email branding GOV.UK Change',
         'Letter branding HM Government Change',
-        'Data Retention email Change'
+        'Data retention email Change'
 
     ]),
 ])

From ce754be464307a807f023da3cb3e3632f398e016 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Wed, 18 Jul 2018 15:31:34 +0100
Subject: [PATCH 05/10] Addressing comments from PR review. Update tests Use a
 table to display the data retention data.

---
 app/main/views/service_settings.py            |  8 ++--
 .../service-settings/data-retention.html      | 24 ++++++-----
 tests/app/main/views/test_service_settings.py | 40 ++++++++++++++++++-
 tests/conftest.py                             | 14 +++++++
 4 files changed, 71 insertions(+), 15 deletions(-)

diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py
index 782c7f3a0..6ed026074 100644
--- a/app/main/views/service_settings.py
+++ b/app/main/views/service_settings.py
@@ -1041,9 +1041,8 @@ def add_data_retention(service_id):
 @login_required
 @user_is_platform_admin
 def edit_data_retention(service_id, data_retention_id):
-    form = ServiceDataRetentionEditForm()
-    if request.method == 'GET':
-        data_retention_item = service_api_client.get_service_data_retention_by_id(service_id, data_retention_id)
+    data_retention_item = service_api_client.get_service_data_retention_by_id(service_id, data_retention_id)
+    form = ServiceDataRetentionEditForm(days_of_retention=data_retention_item['days_of_retention'])
     if form.validate_on_submit():
         service_api_client.update_service_data_retention(service_id, data_retention_id, form.days_of_retention.data)
         return redirect(url_for('.data_retention', service_id=service_id))
@@ -1051,8 +1050,7 @@ def edit_data_retention(service_id, data_retention_id):
         'views/service-settings/data-retention/edit.html',
         form=form,
         data_retention_id=data_retention_id,
-        notification_type=data_retention_item['notification_type'],
-        days_of_retention=data_retention_item['days_of_retention']
+        notification_type=data_retention_item['notification_type']
     )
 
 
diff --git a/app/templates/views/service-settings/data-retention.html b/app/templates/views/service-settings/data-retention.html
index 67322dffc..0802ee123 100644
--- a/app/templates/views/service-settings/data-retention.html
+++ b/app/templates/views/service-settings/data-retention.html
@@ -1,7 +1,7 @@
 {% extends "withnav_template.html" %}
 {% from "components/api-key.html" import api_key %}
 {% from "components/page-footer.html" import page_footer %}
-{% from "components/table.html" import row_group, row, text_field, edit_field, field, boolean_field, list_table with context %}
+{% from "components/table.html" import mapping_table, row_group, row, text_field, edit_field, field, boolean_field, list_table with context %}
 
 {% block service_page_title %}
   Email reply to addresses
@@ -30,15 +30,21 @@
         <span class="hint">You haven’t added any data retention</span>
       </div>
     {% endif %}
-    {% for item in data_retention_settings %}
-      <div class="user-list-item">
+       {% call mapping_table(
+        caption='Data retention',
+        field_headings=['Label', 'Value', 'Action'],
+        field_headings_visible=False,
+        caption_visible=False
+      ) %}
+        {% for item in data_retention_settings %}
+            {% call row() %}
+              {{ text_field(item.notification_type | format_notification_type)}}
+              {{ text_field(item.days_of_retention) }}
+              {{ edit_field('Change', url_for('.link_service_to_organisation', service_id=current_service.id)) }}
+            {% endcall %}
 
-          <p>
-            {{ item.notification_type | format_notification_type }} notifications will be kept for {{ item.days_of_retention }} days
-              <a class="user-list-edit-link" href="{{ url_for('.edit_data_retention', service_id =current_service.id, data_retention_id = item.id) }}">Change</a>
-          </p>
-      </div>
-    {% endfor %}
+        {% endfor %}
+      {% endcall %}
   </div>
 
 {% endblock %}
diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py
index 1b29dcca4..ffbe82db8 100644
--- a/tests/app/main/views/test_service_settings.py
+++ b/tests/app/main/views/test_service_settings.py
@@ -2803,7 +2803,9 @@ def test_show_service_data_retention(
 ):
     response = logged_in_platform_admin_client.get(url_for('main.data_retention', service_id=service_one['id']))
     page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
-    assert normalize_spaces(page.select_one('main p').text) == "Email notifications will be kept for 5 days Change"
+    rows = page.select('tbody tr')
+    assert len(rows) == 1
+    assert normalize_spaces(rows[0].text) == 'Email 5 Change'
 
 
 def test_view_add_service_data_retention(
@@ -2838,6 +2840,7 @@ def test_update_service_data_retention(
         logged_in_platform_admin_client,
         service_one,
         fake_uuid,
+        mock_get_service_data_retention_by_id,
         mock_update_service_data_retention,
 ):
     response = logged_in_platform_admin_client.post(url_for('main.edit_data_retention',
@@ -2850,3 +2853,38 @@ def test_update_service_data_retention(
         'main.data_retention', service_id=service_one['id'], _external=True)
     assert settings_url == response.location
     assert mock_update_service_data_retention.called
+
+
+def test_update_service_data_retention_return_validation_error_for_negative_days_of_retention(
+        logged_in_platform_admin_client,
+        service_one,
+        fake_uuid,
+        mock_get_service_data_retention_by_id,
+        mock_update_service_data_retention,
+):
+    response = logged_in_platform_admin_client.post(url_for('main.edit_data_retention',
+                                                            service_id=service_one['id'],
+                                                            data_retention_id=fake_uuid),
+                                                    data={'days_of_retention': -5}
+                                                    )
+    assert response.status_code == 200
+    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+    error_message = page.find('span', class_='error-message').text.strip()
+    assert error_message == 'Must be between 3 and 90'
+    mock_get_service_data_retention_by_id.called
+    assert not mock_update_service_data_retention.called
+
+
+def test_update_service_data_retention_populates_form(
+        logged_in_platform_admin_client,
+        service_one,
+        fake_uuid,
+        mock_get_service_data_retention_by_id,
+):
+    response = logged_in_platform_admin_client.get(url_for('main.edit_data_retention',
+                                                           service_id=service_one['id'],
+                                                           data_retention_id=fake_uuid)
+                                                   )
+    assert response.status_code == 200
+    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+    assert page.find('input', attrs={'name': 'days_of_retention'})['value'] == '5'
diff --git a/tests/conftest.py b/tests/conftest.py
index bf28929a1..9d6f20ee4 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2684,6 +2684,20 @@ def mock_get_service_data_retention(mocker):
                         return_value=[data])
 
 
+@pytest.fixture(scope='function')
+def mock_get_service_data_retention_by_id(mocker):
+    data = {"id": str(fake_uuid),
+            "service_id": str(fake_uuid),
+            "service_name": "service name",
+            "notification_type": "email",
+            "days_of_retention": 5,
+            "created_at": datetime.now(),
+            "updated_at": None,
+            }
+    return mocker.patch('app.service_api_client.get_service_data_retention_by_id',
+                        return_value=data)
+
+
 @pytest.fixture(scope='function')
 def mock_create_service_data_retention(mocker):
     return mocker.patch('app.service_api_client.create_service_data_retention')

From e89ae1d2ec8875a07a87d70569a765c8bde3aa6a Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Thu, 19 Jul 2018 11:42:34 +0100
Subject: [PATCH 06/10] Added missing assert

---
 tests/app/main/views/test_service_settings.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py
index ffbe82db8..b40794559 100644
--- a/tests/app/main/views/test_service_settings.py
+++ b/tests/app/main/views/test_service_settings.py
@@ -2871,7 +2871,7 @@ def test_update_service_data_retention_return_validation_error_for_negative_days
     page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
     error_message = page.find('span', class_='error-message').text.strip()
     assert error_message == 'Must be between 3 and 90'
-    mock_get_service_data_retention_by_id.called
+    assert mock_get_service_data_retention_by_id.called
     assert not mock_update_service_data_retention.called
 
 

From 0081a70d4b34f38cc5b57f18eff4139ecec6185b Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Thu, 19 Jul 2018 17:09:52 +0100
Subject: [PATCH 07/10] Fix the link on the data-retention/edit.html Adding new
 service_api_client methods to get data for the right amount of days.

---
 app/notify_client/service_api_client.py                | 10 ++++++++--
 .../views/service-settings/data-retention.html         |  2 +-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py
index b2d2f79b5..7f553d983 100644
--- a/app/notify_client/service_api_client.py
+++ b/app/notify_client/service_api_client.py
@@ -41,8 +41,14 @@ class ServiceAPIClient(NotifyAdminAPIClient):
         """
         return self.get('/service/{0}'.format(service_id))
 
-    def get_service_statistics(self, service_id, today_only):
-        return self.get('/service/{0}/statistics'.format(service_id), params={'today_only': today_only})['data']
+    # def get_service_statistics(self, service_id, today_only):
+    #     return self.get('/service/{0}/statistics'.format(service_id), params={'today_only': today_only})['data']
+
+    def get_service_statistics(self, service_id, limit_days=7):
+        return self.get('/service/{0}/statistics'.format(service_id), params={'limit_days': limit_days})['data']
+
+    def get_service_statistics_for_today(self, service_id):
+        return self.get('/service/{0}/statistics'.format(service_id), params={'today_only': True})['data']
 
     def get_services(self, params_dict=None):
         """
diff --git a/app/templates/views/service-settings/data-retention.html b/app/templates/views/service-settings/data-retention.html
index 0802ee123..a48dea2cf 100644
--- a/app/templates/views/service-settings/data-retention.html
+++ b/app/templates/views/service-settings/data-retention.html
@@ -40,7 +40,7 @@
             {% call row() %}
               {{ text_field(item.notification_type | format_notification_type)}}
               {{ text_field(item.days_of_retention) }}
-              {{ edit_field('Change', url_for('.link_service_to_organisation', service_id=current_service.id)) }}
+              {{ edit_field('Change', url_for('.edit_data_retention', service_id=current_service.id, data_retention_id=item.id)) }}
             {% endcall %}
 
         {% endfor %}

From e806cf036ebc40c7135021c2753452cae6ac7ed2 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Wed, 25 Jul 2018 10:47:29 +0100
Subject: [PATCH 08/10] [WIP] New service_api_client to get stats for variable
 days

---
 app/main/views/dashboard.py                     |  2 +-
 app/main/views/jobs.py                          |  3 ++-
 app/main/views/send.py                          |  2 +-
 tests/app/main/views/test_send.py               |  6 +++---
 .../notify_client/test_service_api_client.py    | 17 +++++++++++++----
 tests/conftest.py                               | 14 +++++++++++++-
 6 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py
index b4906b17a..4c9c31d51 100644
--- a/app/main/views/dashboard.py
+++ b/app/main/views/dashboard.py
@@ -321,7 +321,7 @@ def get_dashboard_partials(service_id):
         add_rate_to_job(job)
         for job in job_api_client.get_jobs(service_id, limit_days=7, statuses=statuses_to_display)['data']
     ]
-    stats = service_api_client.get_service_statistics(service_id, today_only=False)
+    stats = service_api_client.get_service_statistics(service_id)
     column_width, max_notifiction_count = get_column_properties(
         number_of_columns=(
             3 if 'letter' in current_service['permissions'] else 2
diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py
index 5527c6367..a5c1b23be 100644
--- a/app/main/views/jobs.py
+++ b/app/main/views/jobs.py
@@ -258,7 +258,8 @@ def get_notifications(service_id, message_type, status_override=None):
             status_filters=get_status_filters(
                 current_service,
                 message_type,
-                service_api_client.get_service_statistics(service_id, today_only=False)
+                service_api_client.get_service_statistics(service_id,
+                                                          limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS'])
             )
         ),
         'notifications': render_template(
diff --git a/app/main/views/send.py b/app/main/views/send.py
index 90c75d01b..c591aa602 100644
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -510,7 +510,7 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_
 
     users = user_api_client.get_users_for_service(service_id=service_id)
 
-    statistics = service_api_client.get_service_statistics(service_id, today_only=True)
+    statistics = service_api_client.get_service_statistics_for_today(service_id)
     remaining_messages = (current_service['message_limit'] - sum(stat['requested'] for stat in statistics.values()))
 
     contents = s3download(service_id, upload_id)
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 6f0ff50bb..bc0e61096 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -384,7 +384,7 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors(
     mock_get_service_template_with_placeholders,
     mock_s3_upload,
     mock_get_users_by_service,
-    mock_get_service_statistics,
+    mock_get_service_statistics_for_today,
     mock_get_job_doesnt_exist,
     fake_uuid,
 ):
@@ -509,7 +509,7 @@ def test_upload_csvfile_with_missing_columns_shows_error(
     mock_get_service_template_with_placeholders,
     mock_s3_upload,
     mock_get_users_by_service,
-    mock_get_service_statistics,
+    mock_get_service_statistics_for_today,
     mock_get_job_doesnt_exist,
     service_one,
     fake_uuid,
@@ -590,7 +590,7 @@ def test_upload_valid_csv_shows_preview_and_table(
     mock_get_live_service,
     mock_get_service_template_with_placeholders,
     mock_get_users_by_service,
-    mock_get_service_statistics,
+    mock_get_service_statistics_for_today,
     mock_get_job_doesnt_exist,
     mock_s3_set_metadata,
     fake_uuid,
diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py
index 7bc4afd4f..b318da388 100644
--- a/tests/app/notify_client/test_service_api_client.py
+++ b/tests/app/notify_client/test_service_api_client.py
@@ -35,15 +35,24 @@ def test_client_gets_service(mocker):
     mock_get.assert_called_once_with('/service/foo')
 
 
-@pytest.mark.parametrize('today_only', [True, False])
-def test_client_gets_service_statistics(mocker, today_only):
+def test_client_gets_service_statistics(mocker):
     client = ServiceAPIClient()
     mock_get = mocker.patch.object(client, 'get', return_value={'data': {'a': 'b'}})
 
-    ret = client.get_service_statistics('foo', today_only)
+    ret = client.get_service_statistics('foo')
 
     assert ret == {'a': 'b'}
-    mock_get.assert_called_once_with('/service/foo/statistics', params={'today_only': today_only})
+    mock_get.assert_called_once_with('/service/foo/statistics')
+
+
+def test_client_gets_service_statistics_for_today(mocker):
+    client = ServiceAPIClient()
+    mock_get = mocker.patch.object(client, 'get', return_value={'data': {'a': 'b'}})
+
+    ret = client.get_service_statistics_for_today('foo')
+
+    assert ret == {'a': 'b'}
+    mock_get.assert_called_once_with('/service/foo/statistics', params={'today_only': True})
 
 
 def test_client_only_updates_allowed_attributes(mocker):
diff --git a/tests/conftest.py b/tests/conftest.py
index 9d6f20ee4..d744da947 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -523,7 +523,7 @@ def mock_get_international_service(mocker, api_user_active):
 
 @pytest.fixture(scope='function')
 def mock_get_service_statistics(mocker, api_user_active):
-    def _get(service_id, today_only):
+    def _get(service_id, limit_days=7):
         return {
             'email': {'requested': 0, 'delivered': 0, 'failed': 0},
             'sms': {'requested': 0, 'delivered': 0, 'failed': 0},
@@ -533,6 +533,18 @@ def mock_get_service_statistics(mocker, api_user_active):
     return mocker.patch('app.service_api_client.get_service_statistics', side_effect=_get)
 
 
+@pytest.fixture(scope='function')
+def mock_get_service_statistics_for_today(mocker, api_user_active):
+    def _get(service_id):
+        return {
+            'email': {'requested': 0, 'delivered': 0, 'failed': 0},
+            'sms': {'requested': 0, 'delivered': 0, 'failed': 0},
+            'letter': {'requested': 0, 'delivered': 0, 'failed': 0}
+        }
+
+    return mocker.patch('app.service_api_client.get_service_statistics_for_today', side_effect=_get)
+
+
 @pytest.fixture(scope='function')
 def mock_get_detailed_services(mocker, fake_uuid):
     service_one = service_json(

From 4b32d9aa0c655650df5c8a1df14c13a64ff29ef9 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Tue, 31 Jul 2018 16:20:10 +0100
Subject: [PATCH 09/10] =?UTF-8?q?Edit,=20don=E2=80=99t=20duplicate,=20exis?=
 =?UTF-8?q?ting=20templates?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 app/main/views/templates.py                   | 6 +++++-
 app/templates/views/edit-email-template.html  | 5 +----
 app/templates/views/edit-letter-template.html | 5 +----
 app/templates/views/edit-sms-template.html    | 5 +----
 4 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/app/main/views/templates.py b/app/main/views/templates.py
index dc23b97c2..732023f65 100644
--- a/app/main/views/templates.py
+++ b/app/main/views/templates.py
@@ -287,6 +287,10 @@ def copy_template(service_id, template_id):
         request.args.get('from_service'),
         str(template_id),
     )['data']
+
+    if request.method == 'POST':
+        return add_service_template(service_id, template['template_type'])
+
     template['template_content'] = template['content']
     template['name'] = 'Copy of ‘{}’'.format(template['name'])
     form = form_objects[template['template_type']](**template)
@@ -458,7 +462,7 @@ def edit_service_template(service_id, template_id):
             form=form,
             template_id=template_id,
             template_type=template['template_type'],
-            heading_action='Edit'
+            heading_action='Edit',
         )
 
 
diff --git a/app/templates/views/edit-email-template.html b/app/templates/views/edit-email-template.html
index 6603d79cf..bc6010a29 100644
--- a/app/templates/views/edit-email-template.html
+++ b/app/templates/views/edit-email-template.html
@@ -13,10 +13,7 @@
       {{ heading_action }} email template
     </h1>
 
-    <form
-      method="post"
-      action="{{ url_for('.add_service_template', service_id=current_service.id, template_type=template_type) }}"
-    >
+    <form method="post">
       <div class="grid-row">
         <div class="column-five-sixths">
           {{ textbox(form.name, width='1-1', hint='Your recipients won’t see this', rows=10) }}
diff --git a/app/templates/views/edit-letter-template.html b/app/templates/views/edit-letter-template.html
index 0fa49c587..804e40339 100644
--- a/app/templates/views/edit-letter-template.html
+++ b/app/templates/views/edit-letter-template.html
@@ -12,10 +12,7 @@
       {{ heading_action }} letter template
     </h1>
 
-    <form
-      method="post"
-      action="{{ url_for('.add_service_template', service_id=current_service.id, template_type=template_type) }}"
-    >
+    <form method="post">
       <div class="grid-row">
         <div class="column-five-sixths">
           {{ textbox(form.name, width='1-1', hint='Your recipients won’t see this', rows=10) }}
diff --git a/app/templates/views/edit-sms-template.html b/app/templates/views/edit-sms-template.html
index 63c929b8e..e7578aee0 100644
--- a/app/templates/views/edit-sms-template.html
+++ b/app/templates/views/edit-sms-template.html
@@ -13,10 +13,7 @@
       {{ heading_action }} text message template
     </h1>
 
-    <form
-      method="post"
-      action="{{ url_for('.add_service_template', service_id=current_service.id, template_type=template_type) }}"
-    >
+    <form method="post">
       <div class="grid-row">
         <div class="column-two-thirds">
           {{ textbox(form.name, width='1-1', hint='Your recipients won’t see this') }}

From 18750585c4c922572d9a400b4efdfa344b94b79d Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Mon, 6 Aug 2018 16:04:31 +0100
Subject: [PATCH 10/10] Reduced the size of the pull request by only dealing
 with adding the data retention form.

---
 app/main/views/jobs.py                          |  3 +--
 app/main/views/send.py                          |  2 +-
 app/notify_client/service_api_client.py         | 10 ++--------
 tests/app/main/views/test_send.py               |  6 +++---
 .../notify_client/test_service_api_client.py    | 17 ++++-------------
 tests/conftest.py                               | 14 +-------------
 6 files changed, 12 insertions(+), 40 deletions(-)

diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py
index a932ff1cb..265f0a0d5 100644
--- a/app/main/views/jobs.py
+++ b/app/main/views/jobs.py
@@ -265,8 +265,7 @@ def get_notifications(service_id, message_type, status_override=None):
             status_filters=get_status_filters(
                 current_service,
                 message_type,
-                service_api_client.get_service_statistics(service_id,
-                                                          limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS'])
+                service_api_client.get_service_statistics(service_id, today_only=False)
             )
         ),
         'notifications': render_template(
diff --git a/app/main/views/send.py b/app/main/views/send.py
index 12273ce31..785b277a5 100644
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -511,7 +511,7 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_
     users = user_api_client.get_users_for_service(service_id=service_id)
 
     statistics = service_api_client.get_service_statistics(service_id, today_only=True)
-    remaining_messages = (current_service['message_limit'] - sum(stat['requested'] for stat in statistics.values()))
+    remaining_messages = (current_service.message_limit - sum(stat['requested'] for stat in statistics.values()))
 
     contents = s3download(service_id, upload_id)
 
diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py
index 7f553d983..b2d2f79b5 100644
--- a/app/notify_client/service_api_client.py
+++ b/app/notify_client/service_api_client.py
@@ -41,14 +41,8 @@ class ServiceAPIClient(NotifyAdminAPIClient):
         """
         return self.get('/service/{0}'.format(service_id))
 
-    # def get_service_statistics(self, service_id, today_only):
-    #     return self.get('/service/{0}/statistics'.format(service_id), params={'today_only': today_only})['data']
-
-    def get_service_statistics(self, service_id, limit_days=7):
-        return self.get('/service/{0}/statistics'.format(service_id), params={'limit_days': limit_days})['data']
-
-    def get_service_statistics_for_today(self, service_id):
-        return self.get('/service/{0}/statistics'.format(service_id), params={'today_only': True})['data']
+    def get_service_statistics(self, service_id, today_only):
+        return self.get('/service/{0}/statistics'.format(service_id), params={'today_only': today_only})['data']
 
     def get_services(self, params_dict=None):
         """
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 4ebddb45c..72ef0af98 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -384,7 +384,7 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors(
     mock_get_service_template_with_placeholders,
     mock_s3_upload,
     mock_get_users_by_service,
-    mock_get_service_statistics_for_today,
+    mock_get_service_statistics,
     mock_get_job_doesnt_exist,
     fake_uuid,
 ):
@@ -509,7 +509,7 @@ def test_upload_csvfile_with_missing_columns_shows_error(
     mock_get_service_template_with_placeholders,
     mock_s3_upload,
     mock_get_users_by_service,
-    mock_get_service_statistics_for_today,
+    mock_get_service_statistics,
     mock_get_job_doesnt_exist,
     service_one,
     fake_uuid,
@@ -590,7 +590,7 @@ def test_upload_valid_csv_shows_preview_and_table(
     mock_get_live_service,
     mock_get_service_template_with_placeholders,
     mock_get_users_by_service,
-    mock_get_service_statistics_for_today,
+    mock_get_service_statistics,
     mock_get_job_doesnt_exist,
     mock_s3_set_metadata,
     fake_uuid,
diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py
index b318da388..7bc4afd4f 100644
--- a/tests/app/notify_client/test_service_api_client.py
+++ b/tests/app/notify_client/test_service_api_client.py
@@ -35,24 +35,15 @@ def test_client_gets_service(mocker):
     mock_get.assert_called_once_with('/service/foo')
 
 
-def test_client_gets_service_statistics(mocker):
+@pytest.mark.parametrize('today_only', [True, False])
+def test_client_gets_service_statistics(mocker, today_only):
     client = ServiceAPIClient()
     mock_get = mocker.patch.object(client, 'get', return_value={'data': {'a': 'b'}})
 
-    ret = client.get_service_statistics('foo')
+    ret = client.get_service_statistics('foo', today_only)
 
     assert ret == {'a': 'b'}
-    mock_get.assert_called_once_with('/service/foo/statistics')
-
-
-def test_client_gets_service_statistics_for_today(mocker):
-    client = ServiceAPIClient()
-    mock_get = mocker.patch.object(client, 'get', return_value={'data': {'a': 'b'}})
-
-    ret = client.get_service_statistics_for_today('foo')
-
-    assert ret == {'a': 'b'}
-    mock_get.assert_called_once_with('/service/foo/statistics', params={'today_only': True})
+    mock_get.assert_called_once_with('/service/foo/statistics', params={'today_only': today_only})
 
 
 def test_client_only_updates_allowed_attributes(mocker):
diff --git a/tests/conftest.py b/tests/conftest.py
index 0864e9cc9..aafed172e 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -523,7 +523,7 @@ def mock_get_international_service(mocker, api_user_active):
 
 @pytest.fixture(scope='function')
 def mock_get_service_statistics(mocker, api_user_active):
-    def _get(service_id, limit_days=7):
+    def _get(service_id, today_only):
         return {
             'email': {'requested': 0, 'delivered': 0, 'failed': 0},
             'sms': {'requested': 0, 'delivered': 0, 'failed': 0},
@@ -533,18 +533,6 @@ def mock_get_service_statistics(mocker, api_user_active):
     return mocker.patch('app.service_api_client.get_service_statistics', side_effect=_get)
 
 
-@pytest.fixture(scope='function')
-def mock_get_service_statistics_for_today(mocker, api_user_active):
-    def _get(service_id):
-        return {
-            'email': {'requested': 0, 'delivered': 0, 'failed': 0},
-            'sms': {'requested': 0, 'delivered': 0, 'failed': 0},
-            'letter': {'requested': 0, 'delivered': 0, 'failed': 0}
-        }
-
-    return mocker.patch('app.service_api_client.get_service_statistics_for_today', side_effect=_get)
-
-
 @pytest.fixture(scope='function')
 def mock_get_detailed_services(mocker, fake_uuid):
     service_one = service_json(