From 58b2a8a825f090aefabddd7670e2f8748d0fab7a Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Wed, 24 Aug 2016 09:34:14 +0100
Subject: [PATCH] Make email branding settings a table

So that it matches the other settings on this page
---
 app/main/views/service_settings.py            |   9 +-
 app/notify_client/organisations_client.py     |   3 +
 app/templates/views/service-settings.html     |  26 ++-
 tests/app/main/views/test_service_settings.py | 150 ++++++++++--------
 tests/conftest.py                             |  17 ++
 5 files changed, 133 insertions(+), 72 deletions(-)

diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py
index 301aafd61..3f90d2bdf 100644
--- a/app/main/views/service_settings.py
+++ b/app/main/views/service_settings.py
@@ -34,7 +34,14 @@ from app import user_api_client, current_service, organisations_client
 @login_required
 @user_has_permissions('manage_settings', admin_override=True)
 def service_settings(service_id):
-    return render_template('views/service-settings.html')
+    if current_service['organisation']:
+        organisation = organisations_client.get_organisation(current_service['organisation'])['organisation']
+    else:
+        organisation = None
+    return render_template(
+        'views/service-settings.html',
+        organisation=organisation
+    )
 
 
 @main.route("/services/<service_id>/service-settings/name", methods=['GET', 'POST'])
diff --git a/app/notify_client/organisations_client.py b/app/notify_client/organisations_client.py
index 3d39decf7..21a4ae36e 100644
--- a/app/notify_client/organisations_client.py
+++ b/app/notify_client/organisations_client.py
@@ -15,5 +15,8 @@ class OrganisationsClient(BaseAPIClient):
         self.client_id = app.config['ADMIN_CLIENT_USER_NAME']
         self.secret = app.config['ADMIN_CLIENT_SECRET']
 
+    def get_organisation(self, id):
+        return self.get(url='/organisation/{}'.format(id))
+
     def get_organisations(self):
         return self.get(url='/organisation')['organisations']
diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html
index 9f4afcfab..053e950f5 100644
--- a/app/templates/views/service-settings.html
+++ b/app/templates/views/service-settings.html
@@ -60,12 +60,26 @@
 
       <h2 class="heading-medium">Platform admin settings</h2>
 
-      {{ browse_list([
-        {
-          'title': 'Email branding',
-          'link': url_for('.service_set_branding_and_org', service_id=current_service.id)
-        }
-      ]) }}
+      {% call mapping_table(
+        caption='Settings',
+        field_headings=['Label', 'Value', 'Action'],
+        field_headings_visible=False,
+        caption_visible=False
+      ) %}
+        {% call row() %}
+          {{ text_field('Email branding' )}}
+          {% call field() %}
+            {% if current_service.branding == 'govuk' %}
+              GOV.UK
+            {% elif current_service.branding == 'both' %}
+              GOV.UK and {{ organisation.name if organisation else None }}
+            {% elif current_service.branding == 'org' %}
+              Only {{ organisation.name if organisation else None }}
+            {% endif %}
+          {% endcall %}
+          {{ edit_field('Change', url_for('.service_set_branding_and_org', service_id=current_service.id)) }}
+        {% endcall %}
+      {% endcall %}
 
       <ul>
         <li class="bottom-gutter">
diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py
index 6f9585f62..08080b599 100644
--- a/tests/app/main/views/test_service_settings.py
+++ b/tests/app/main/views/test_service_settings.py
@@ -13,7 +13,8 @@ def test_should_show_overview(
     app_,
     active_user_with_permissions,
     mocker,
-    service_one
+    service_one,
+    mock_get_organisation
 ):
     with app_.test_request_context(), app_.test_client() as client:
         client.login(active_user_with_permissions, mocker, service_one)
@@ -36,7 +37,8 @@ def test_should_show_overview_for_service_with_more_things_set(
     app_,
     active_user_with_permissions,
     mocker,
-    service_with_reply_to_addresses
+    service_with_reply_to_addresses,
+    mock_get_organisation
 ):
     with app_.test_request_context(), app_.test_client() as client:
         client.login(active_user_with_permissions, mocker, service_with_reply_to_addresses)
@@ -91,14 +93,17 @@ def test_should_redirect_after_change_service_name(app_,
         assert mock_get_services.called
 
 
-def test_switch_service_to_live(app_,
-                                service_one,
-                                mock_login,
-                                mock_get_user,
-                                active_user_with_permissions,
-                                mock_get_service,
-                                mock_update_service,
-                                mock_has_permissions):
+def test_switch_service_to_live(
+    app_,
+    service_one,
+    mock_login,
+    mock_get_user,
+    active_user_with_permissions,
+    mock_get_service,
+    mock_update_service,
+    mock_has_permissions,
+    mock_get_organisation
+):
     with app_.test_request_context():
         with app_.test_client() as client:
             client.login(active_user_with_permissions)
@@ -115,14 +120,17 @@ def test_switch_service_to_live(app_,
         )
 
 
-def test_switch_service_to_restricted(app_,
-                                      service_one,
-                                      mock_login,
-                                      mock_get_user,
-                                      active_user_with_permissions,
-                                      mock_get_live_service,
-                                      mock_update_service,
-                                      mock_has_permissions):
+def test_switch_service_to_restricted(
+    app_,
+    service_one,
+    mock_login,
+    mock_get_user,
+    active_user_with_permissions,
+    mock_get_live_service,
+    mock_update_service,
+    mock_has_permissions,
+    mock_get_organisation
+):
     with app_.test_request_context():
         with app_.test_client() as client:
             client.login(active_user_with_permissions)
@@ -176,12 +184,15 @@ def test_should_show_service_name_confirmation(app_,
         app.service_api_client.get_service.assert_called_with(service_one['id'])
 
 
-def test_should_redirect_after_service_name_confirmation(app_,
-                                                         active_user_with_permissions,
-                                                         service_one,
-                                                         mocker,
-                                                         mock_update_service,
-                                                         mock_verify_password):
+def test_should_redirect_after_service_name_confirmation(
+    app_,
+    active_user_with_permissions,
+    service_one,
+    mocker,
+    mock_update_service,
+    mock_verify_password,
+    mock_get_organisation
+):
     with app_.test_request_context():
         with app_.test_client() as client:
             client.login(active_user_with_permissions, mocker, service_one)
@@ -250,12 +261,13 @@ def test_should_show_request_to_go_live(app_,
 
 
 def test_should_redirect_after_request_to_go_live(
-        app_,
-        api_user_active,
-        mock_get_user,
-        mock_get_service,
-        mock_has_permissions,
-        mocker
+    app_,
+    api_user_active,
+    mock_get_user,
+    mock_get_service,
+    mock_has_permissions,
+    mock_get_organisation,
+    mocker
 ):
     mock_post = mocker.patch(
         'app.main.views.feedback.requests.post',
@@ -290,12 +302,12 @@ def test_should_redirect_after_request_to_go_live(
 
 
 def test_log_error_on_request_to_go_live(
-        app_,
-        api_user_active,
-        mock_get_user,
-        mock_get_service,
-        mock_has_permissions,
-        mocker
+    app_,
+    api_user_active,
+    mock_get_user,
+    mock_get_service,
+    mock_has_permissions,
+    mocker
 ):
     mock_post = mocker.patch(
         'app.main.views.service_settings.requests.post',
@@ -407,7 +419,7 @@ def test_should_redirect_delete_confirmation(app_,
         assert mock_delete_service.called
 
 
-def test_route_permissions(mocker, app_, api_user_active, service_one):
+def test_route_permissions(mocker, app_, api_user_active, service_one, mock_get_organisation):
     routes = [
         'main.service_settings',
         'main.service_name_change',
@@ -428,7 +440,7 @@ def test_route_permissions(mocker, app_, api_user_active, service_one):
                 service_one)
 
 
-def test_route_invalid_permissions(mocker, app_, api_user_active, service_one):
+def test_route_invalid_permissions(mocker, app_, api_user_active, service_one, mock_get_organisation):
     routes = [
         'main.service_settings',
         'main.service_name_change',
@@ -451,7 +463,7 @@ def test_route_invalid_permissions(mocker, app_, api_user_active, service_one):
                 service_one)
 
 
-def test_route_for_platform_admin(mocker, app_, platform_admin_user, service_one):
+def test_route_for_platform_admin(mocker, app_, platform_admin_user, service_one, mock_get_organisation):
     routes = [
         'main.service_settings',
         'main.service_name_change',
@@ -490,11 +502,13 @@ def test_route_for_platform_admin_update_service(mocker, app_, platform_admin_us
 
 
 def test_set_reply_to_email_address(
-        app_,
-        active_user_with_permissions,
-        mocker,
-        mock_update_service,
-        service_one):
+    app_,
+    active_user_with_permissions,
+    mocker,
+    mock_update_service,
+    service_one,
+    mock_get_organisation
+):
     with app_.test_request_context():
         with app_.test_client() as client:
             client.login(active_user_with_permissions, mocker, service_one)
@@ -583,14 +597,16 @@ def test_switch_service_from_research_mode_to_normal(
 
 
 def test_shows_research_mode_indicator(
-        app_,
-        service_one,
-        mock_login,
-        mock_get_user,
-        active_user_with_permissions,
-        mock_get_service,
-        mock_has_permissions,
-        mocker):
+    app_,
+    service_one,
+    mock_login,
+    mock_get_user,
+    active_user_with_permissions,
+    mock_get_service,
+    mock_has_permissions,
+    mock_get_organisation,
+    mocker
+):
     with app_.test_request_context():
         with app_.test_client() as client:
             service = service_json(
@@ -615,14 +631,16 @@ def test_shows_research_mode_indicator(
 
 
 def test_does_not_show_research_mode_indicator(
-        app_,
-        service_one,
-        mock_login,
-        mock_get_user,
-        active_user_with_permissions,
-        mock_get_service,
-        mock_has_permissions,
-        mocker):
+    app_,
+    service_one,
+    mock_login,
+    mock_get_user,
+    active_user_with_permissions,
+    mock_get_service,
+    mock_has_permissions,
+    mock_get_organisation,
+    mocker
+):
     with app_.test_request_context():
         with app_.test_client() as client:
             client.login(active_user_with_permissions)
@@ -635,11 +653,13 @@ def test_does_not_show_research_mode_indicator(
 
 
 def test_set_text_message_sender(
-        app_,
-        active_user_with_permissions,
-        mocker,
-        mock_update_service,
-        service_one):
+    app_,
+    active_user_with_permissions,
+    mocker,
+    mock_update_service,
+    service_one,
+    mock_get_organisation
+):
     with app_.test_request_context():
         with app_.test_client() as client:
             client.login(active_user_with_permissions, mocker, service_one)
diff --git a/tests/conftest.py b/tests/conftest.py
index 68b43b34a..8d3190e64 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -1144,3 +1144,20 @@ def mock_get_organisations(mocker):
     return mocker.patch(
         'app.organisations_client.get_organisations', side_effect=_get_organisations
     )
+
+
+@pytest.fixture(scope='function')
+def mock_get_organisation(mocker):
+    def _get_organisation(id):
+        return {
+            'organisation': {
+                'logo': 'example.png',
+                'name': 'Organisation name',
+                'id': 'organisation-id',
+                'colour': '#f00'
+            }
+        }
+
+    return mocker.patch(
+        'app.organisations_client.get_organisation', side_effect=_get_organisation
+    )