From 2f711b52ea0dd1f3fb7504c8544f29977e55c608 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 7 Jun 2019 10:16:21 +0100
Subject: [PATCH] Refactor choose service page into macros

This makes it easier to see the logic controlling which services are
shown where because the repetitive loops have been factored out.
---
 app/templates/views/choose-account.html       | 104 +++++++++---------
 .../views/accounts/test_choose_accounts.py    |   2 +-
 2 files changed, 56 insertions(+), 50 deletions(-)

diff --git a/app/templates/views/choose-account.html b/app/templates/views/choose-account.html
index fb9da0802..d1d0bbeb4 100644
--- a/app/templates/views/choose-account.html
+++ b/app/templates/views/choose-account.html
@@ -1,5 +1,29 @@
 {% extends "withoutnav_template.html" %}
 
+{% macro service_list(heading, show_heading, organisations=[], services=[]) %}
+  {% if show_heading %}
+    <h2 class="heading-small">
+      {{ heading }}
+    </h2>
+  {% endif %}
+  <ul>
+  {% for org in organisations %}
+    <li class="browse-list-item">
+      <a href="{{ url_for('.organisation_dashboard', org_id=org.id) }}" class="browse-list-link">{{ org.name }}</a>
+      <p class="browse-list-hint">
+        {{ org.live_services|length }}
+        live service{% if org.live_services|length != 1 %}s{% endif %}
+      </p>
+    </li>
+  {% endfor %}
+  {% for service in services %}
+    <li class="browse-list-item">
+      <a href="{{ url_for('.service_dashboard', service_id=service.id) }}" class="browse-list-link">{{ service.name }}</a>
+    </li>
+  {% endfor %}
+  </ul>
+{% endmacro %}
+
 {% block per_page_title %}
   Choose service
 {% endblock %}
@@ -10,58 +34,40 @@
     Choose service
   </h1>
   <nav class="browse-list">
-    <ul>
-      {% if current_user.platform_admin %}
+
+    {% if current_user.platform_admin %}
+      <h2>Platform admin</h2>
+      <ul>
         <li class="browse-list-item">
           <a href="{{ url_for('.organisations') }}" class="browse-list-link">All organisations</a>
         </li>
-        <div class ="keyline-block"></div>
-      {% endif %}
-      {% if current_user.organisations %}
-        {% for org in current_user.organisations %}
-          <li class="browse-list-item">
-            <a href="{{ url_for('.organisation_dashboard', org_id=org.id) }}" class="browse-list-link">{{ org.name }}</a>
-            <p class="browse-list-hint">
-              {{ org.live_services|length }}
-              live service{% if org.live_services|length != 1 %}s{% endif %}
-            </p>
-          </li>
-          <div class ="keyline-block"></div>
-        {% endfor %}
-        {% for item in current_user.live_services_not_belonging_to_users_organisations %}
-          <li class="browse-list-item">
-            <a href="{{ url_for('.service_dashboard', service_id=item.id) }}" class="browse-list-link">{{ item.name }}</a>
-          </li>
-        {% endfor %}
-      {% else %}
-        {% if current_user.trial_mode_services and current_user.live_services %}
-          </ul>
-          <h2 class="heading-small">
-            Live services
-          </h2>
-          <ul>
-        {% endif %}
-        {% for service in current_user.live_services %}
-          <li class="browse-list-item">
-            <a href="{{ url_for('.service_dashboard', service_id=service.id) }}" class="browse-list-link">{{ service.name }}</a>
-          </li>
-        {% endfor %}
-      {% endif %}
-      {% if current_user.trial_mode_services %}
-        {% if organisations or current_user.live_services %}
-          </ul>
-          <h2 class="heading-small">
-            Trial mode services
-          </h2>
-          <ul>
-        {% endif %}
-        {% for service in current_user.trial_mode_services %}
-          <li class="browse-list-item">
-            <a href="{{ url_for('.service_dashboard', service_id=service.id) }}" class="browse-list-link">{{ service.name }}</a>
-          </li>
-        {% endfor %}
-      {% endif %}
-    </ul>
+      <ul>
+      <div class ="keyline-block"></div>
+    {% endif %}
+
+    {% if current_user.organisations %}
+      {{ service_list(
+        heading='Live services',
+        show_heading=current_user.trial_mode_services,
+        organisations=current_user.organisations,
+        services=current_user.live_services_not_belonging_to_users_organisations
+      ) }}
+    {% else %}
+      {{ service_list(
+        heading='Live services',
+        show_heading=(current_user.trial_mode_services and current_user.live_services),
+        services=current_user.live_services
+      ) }}
+    {% endif %}
+
+    {% if current_user.trial_mode_services %}
+      {{ service_list(
+        heading='Trial mode services',
+        show_heading=(current_user.organisations or current_user.live_services),
+        services=current_user.trial_mode_services
+      ) }}
+    {% endif %}
+
   </nav>
   {% if can_add_service %}
     <div class="js-stick-at-bottom-when-scrolling">
diff --git a/tests/app/main/views/accounts/test_choose_accounts.py b/tests/app/main/views/accounts/test_choose_accounts.py
index d26dacf42..ead1a283c 100644
--- a/tests/app/main/views/accounts/test_choose_accounts.py
+++ b/tests/app/main/views/accounts/test_choose_accounts.py
@@ -89,7 +89,7 @@ def test_choose_account_should_show_choose_accounts_page(
     assert outer_list_items[4].a.text == 'service_2'
     assert outer_list_items[4].a['href'] == url_for('.service_dashboard', service_id='s2')
 
-    # orphaned live services
+    # orphaned trial services
     trial_services_list_items = page.select('nav ul')[1].select('li')
     assert len(trial_services_list_items) == 2
     assert trial_services_list_items[0].a.text == 'org_service_3'