From 4d4c9c0db29ee851599afde70597659567cdaa81 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Mon, 27 Sep 2021 14:44:29 +0100
Subject: [PATCH 1/7] Make services natively sortable

---
 app/models/organisation.py | 3 +++
 app/models/service.py      | 3 +++
 app/models/user.py         | 8 ++------
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/app/models/organisation.py b/app/models/organisation.py
index 5ddffaf88..d9480f9c3 100644
--- a/app/models/organisation.py
+++ b/app/models/organisation.py
@@ -59,6 +59,9 @@ class Organisation(JSONModel):
             return cls({})
         return cls(organisations_client.get_organisation(org_id))
 
+    def __lt__(self, other):
+        return self.name.lower() < other.name.lower()
+
     @classmethod
     def from_domain(cls, domain):
         return cls(organisations_client.get_organisation_by_domain(domain))
diff --git a/app/models/service.py b/app/models/service.py
index b6012c5ad..659e3e8e9 100644
--- a/app/models/service.py
+++ b/app/models/service.py
@@ -77,6 +77,9 @@ class Service(JSONModel):
     def from_id(cls, service_id):
         return cls(service_api_client.get_service(service_id)['data'])
 
+    def __lt__(self, other):
+        return self.name.lower() < other.name.lower()
+
     @property
     def permissions(self):
         return self._dict.get('permissions', self.TEMPLATE_TYPES)
diff --git a/app/models/user.py b/app/models/user.py
index f314d278c..43cd9a231 100644
--- a/app/models/user.py
+++ b/app/models/user.py
@@ -290,16 +290,12 @@ class User(JSONModel, UserMixin):
     def orgs_and_services(self):
         return user_api_client.get_organisations_and_services_for_user(self.id)
 
-    @staticmethod
-    def sort_services(services):
-        return sorted(services, key=lambda service: service.name.lower())
-
     @property
     def services(self):
         from app.models.service import Service
-        return self.sort_services([
+        return sorted(
             Service(service) for service in self.orgs_and_services['services']
-        ])
+        )
 
     @property
     def services_with_organisation(self):

From 9281ca7d50986c8a19940313f0b4283058a3b518 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Mon, 27 Sep 2021 15:20:56 +0100
Subject: [PATCH 2/7] Sort services and orgs in presentation layer
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The model layer shouldn’t need to be concerned with sorting. For
services this means we can make a `SerialisedModelCollection` rather
than writing a manual loop.
---
 app/models/user.py                      | 4 ++--
 app/templates/views/choose-account.html | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/models/user.py b/app/models/user.py
index 43cd9a231..39dcd5d86 100644
--- a/app/models/user.py
+++ b/app/models/user.py
@@ -293,9 +293,9 @@ class User(JSONModel, UserMixin):
     @property
     def services(self):
         from app.models.service import Service
-        return sorted(
+        return [
             Service(service) for service in self.orgs_and_services['services']
-        )
+        ]
 
     @property
     def services_with_organisation(self):
diff --git a/app/templates/views/choose-account.html b/app/templates/views/choose-account.html
index c5f3f78df..6c96efb84 100644
--- a/app/templates/views/choose-account.html
+++ b/app/templates/views/choose-account.html
@@ -19,7 +19,7 @@
   {% else %}
     <ul>
   {% endif %}
-  {% for org in organisations %}
+  {% for org in organisations|sort %}
     <li class="browse-list-item">
       <a href="{{ url_for('.organisation_dashboard', org_id=org.id) }}" class="govuk-link govuk-link--no-visited-state">{{ org.name }}</a>
       <p class="browse-list-hint">
@@ -28,7 +28,7 @@
       </p>
     </li>
   {% endfor %}
-  {% for service in services %}
+  {% for service in services|sort %}
     <li class="browse-list-item">
       <a href="{{ url_for('.service_dashboard', service_id=service.id) }}" class="govuk-link govuk-link--no-visited-state">{{ service.name }}</a>
     </li>

From cbbc58e6490f9bc18204f8718ac7fd21c7d20ea8 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Mon, 27 Sep 2021 15:20:56 +0100
Subject: [PATCH 3/7] Make a model collection for services

This is tidier than having a manual loop.
---
 app/models/service.py | 5 +++++
 app/models/user.py    | 6 ++----
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/app/models/service.py b/app/models/service.py
index 659e3e8e9..ce77234c5 100644
--- a/app/models/service.py
+++ b/app/models/service.py
@@ -1,4 +1,5 @@
 from flask import abort, current_app
+from notifications_utils.serialised_model import SerialisedModelCollection
 from werkzeug.utils import cached_property
 
 from app.models import JSONModel
@@ -710,3 +711,7 @@ class Service(JSONModel):
     @property
     def contact_lists(self):
         return ContactLists(self.id)
+
+
+class Services(SerialisedModelCollection):
+    model = Service
diff --git a/app/models/user.py b/app/models/user.py
index 39dcd5d86..fe427fc54 100644
--- a/app/models/user.py
+++ b/app/models/user.py
@@ -292,10 +292,8 @@ class User(JSONModel, UserMixin):
 
     @property
     def services(self):
-        from app.models.service import Service
-        return [
-            Service(service) for service in self.orgs_and_services['services']
-        ]
+        from app.models.service import Services
+        return Services(self.orgs_and_services['services'])
 
     @property
     def services_with_organisation(self):

From 029682d5616c8f9481e35eefca785cf2152df9fd Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Tue, 28 Sep 2021 09:02:15 +0100
Subject: [PATCH 4/7] Rename model to AllOrganisations
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This makes it clearer that this model collection isn’t the organisations
for a user or a service or some other entity, like most model
collections are.

It will also lets us make a separate Organisations model, without the
name conflicting.
---
 app/main/views/choose_account.py                         | 4 ++--
 app/main/views/organisations.py                          | 6 +++---
 app/models/organisation.py                               | 2 +-
 tests/app/main/views/organisations/test_organisations.py | 6 +++---
 tests/conftest.py                                        | 2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/app/main/views/choose_account.py b/app/main/views/choose_account.py
index 1c26a0b8f..2509b111d 100644
--- a/app/main/views/choose_account.py
+++ b/app/main/views/choose_account.py
@@ -3,7 +3,7 @@ from flask_login import current_user
 
 from app import status_api_client
 from app.main import main
-from app.models.organisation import Organisations
+from app.models.organisation import AllOrganisations
 from app.utils import PermanentRedirect
 from app.utils.user import user_is_logged_in
 
@@ -24,7 +24,7 @@ def choose_account():
     org_count, live_service_count = None, None
     if current_user.platform_admin:
         org_count, live_service_count = (
-            len(Organisations()),
+            len(AllOrganisations()),
             status_api_client.get_count_of_live_services_and_organisations()['services'],
         )
     return render_template(
diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py
index 867337266..ac4a3e221 100644
--- a/app/main/views/organisations.py
+++ b/app/main/views/organisations.py
@@ -41,7 +41,7 @@ from app.main.views.dashboard import (
     requested_and_current_financial_year,
 )
 from app.main.views.service_settings import get_branding_as_value_and_label
-from app.models.organisation import Organisation, Organisations
+from app.models.organisation import AllOrganisations, Organisation
 from app.models.user import InvitedOrgUser, User
 from app.utils.user import user_has_permissions, user_is_platform_admin
 
@@ -51,7 +51,7 @@ from app.utils.user import user_has_permissions, user_is_platform_admin
 def organisations():
     return render_template(
         'views/organisations/index.html',
-        organisations=Organisations(),
+        organisations=AllOrganisations(),
         search_form=SearchByNameForm(),
     )
 
@@ -116,7 +116,7 @@ def add_organisation_from_nhs_local_service(service_id):
 
     form = AddNHSLocalOrganisationForm(organisation_choices=[
         (organisation.id, organisation.name)
-        for organisation in Organisations()
+        for organisation in AllOrganisations()
         if organisation.organisation_type == Organisation.TYPE_NHS_LOCAL
     ])
 
diff --git a/app/models/organisation.py b/app/models/organisation.py
index d9480f9c3..4dfacfc8e 100644
--- a/app/models/organisation.py
+++ b/app/models/organisation.py
@@ -204,6 +204,6 @@ class Organisation(JSONModel):
         return organisations_client.get_services_and_usage(self.id, financial_year)
 
 
-class Organisations(ModelList):
+class AllOrganisations(ModelList):
     client_method = organisations_client.get_organisations
     model = Organisation
diff --git a/tests/app/main/views/organisations/test_organisations.py b/tests/app/main/views/organisations/test_organisations.py
index c690d5301..ab0e5a82b 100644
--- a/tests/app/main/views/organisations/test_organisations.py
+++ b/tests/app/main/views/organisations/test_organisations.py
@@ -26,7 +26,7 @@ def test_organisation_page_shows_all_organisations(
     ]
 
     get_organisations = mocker.patch(
-        'app.models.organisation.Organisations.client_method', return_value=orgs
+        'app.models.organisation.AllOrganisations.client_method', return_value=orgs
     )
     response = platform_admin_client.get(
         url_for('.organisations')
@@ -274,7 +274,7 @@ def test_nhs_local_can_create_own_organisations(
 ):
     mocker.patch('app.organisations_client.get_organisation', return_value=organisation)
     mocker.patch(
-        'app.models.organisation.Organisations.client_method',
+        'app.models.organisation.AllOrganisations.client_method',
         return_value=[
             organisation_json('t1', 'Trust 1', organisation_type='nhs_local'),
             organisation_json('t2', 'Trust 2', organisation_type='nhs_local'),
@@ -405,7 +405,7 @@ def test_nhs_local_assigns_to_selected_organisation(
     mock_update_service_organisation,
 ):
     mocker.patch(
-        'app.models.organisation.Organisations.client_method',
+        'app.models.organisation.AllOrganisations.client_method',
         return_value=[
             organisation_json(ORGANISATION_ID, 'Trust 1', organisation_type='nhs_local'),
         ],
diff --git a/tests/conftest.py b/tests/conftest.py
index f45a38919..5ae94b09c 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -3119,7 +3119,7 @@ def mock_get_organisations(mocker):
         ]
 
     mocker.patch(
-        'app.models.organisation.Organisations.client_method',
+        'app.models.organisation.AllOrganisations.client_method',
         side_effect=_get_organisations,
     )
 

From 8934e402e8313682ebdf570e1a746eef6ae39a62 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Tue, 28 Sep 2021 09:04:21 +0100
Subject: [PATCH 5/7] Make a model collection for organisations
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This makes returning a user’s organisations consistent with how we
return their services.
---
 app/models/organisation.py | 9 ++++++---
 app/models/user.py         | 7 ++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/app/models/organisation.py b/app/models/organisation.py
index 4dfacfc8e..bfb0fe9f2 100644
--- a/app/models/organisation.py
+++ b/app/models/organisation.py
@@ -1,7 +1,7 @@
 from flask import abort
 from werkzeug.utils import cached_property
 
-from app.models import JSONModel, ModelList
+from app.models import JSONModel, ModelList, SerialisedModelCollection
 from app.notify_client.email_branding_client import email_branding_client
 from app.notify_client.letter_branding_client import letter_branding_client
 from app.notify_client.organisations_api_client import organisations_client
@@ -204,6 +204,9 @@ class Organisation(JSONModel):
         return organisations_client.get_services_and_usage(self.id, financial_year)
 
 
-class AllOrganisations(ModelList):
-    client_method = organisations_client.get_organisations
+class Organisations(SerialisedModelCollection):
     model = Organisation
+
+
+class AllOrganisations(ModelList, Organisations):
+    client_method = organisations_client.get_organisations
diff --git a/app/models/user.py b/app/models/user.py
index fe427fc54..ce7e41842 100644
--- a/app/models/user.py
+++ b/app/models/user.py
@@ -11,7 +11,7 @@ from app.event_handlers import (
     create_set_user_permissions_event,
 )
 from app.models import JSONModel, ModelList
-from app.models.organisation import Organisation
+from app.models.organisation import Organisation, Organisations
 from app.models.webauthn_credential import WebAuthnCredentials
 from app.notify_client import InviteTokenError
 from app.notify_client.invite_api_client import invite_api_client
@@ -320,10 +320,7 @@ class User(JSONModel, UserMixin):
 
     @property
     def organisations(self):
-        return [
-            Organisation(organisation)
-            for organisation in self.orgs_and_services['organisations']
-        ]
+        return Organisations(self.orgs_and_services['organisations'])
 
     @property
     def organisation_ids(self):

From 18dfc448997ea2ddf964c55eed9a802f91f7432c Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Thu, 11 Nov 2021 14:23:07 +0000
Subject: [PATCH 6/7] Sort services on user information page
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Since the model doesn’t sort automatically we need to do this in the
presentation layer now.
---
 app/templates/views/find-users/user-information.html | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/templates/views/find-users/user-information.html b/app/templates/views/find-users/user-information.html
index 0b93d7e9f..2e9ef6569 100644
--- a/app/templates/views/find-users/user-information.html
+++ b/app/templates/views/find-users/user-information.html
@@ -17,7 +17,7 @@
       <nav class="browse-list">
         {% if user.live_services %}
           <ul>
-          {% for service in user.live_services %}
+          {% for service in user.live_services|sort %}
             <li class="browse-list-item">
               <a class="govuk-link govuk-link--no-visited-state browse-list-hint" href={{url_for('.service_dashboard', service_id=service.id)}}>{{ service.name }}</a>
             </li>
@@ -33,7 +33,7 @@
       <nav class="browse-list">
         {% if user.trial_mode_services %}
           <ul>
-          {% for service in user.trial_mode_services %}
+          {% for service in user.trial_mode_services|sort %}
             <li class="browse-list-item">
               <a class="govuk-link govuk-link--no-visited-state browse-list-hint" href={{url_for('.service_dashboard', service_id=service.id)}}>{{ service.name }}</a>
             </li>

From 0c2b586e40c614edd844be1c7375203b30d5a847 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Thu, 11 Nov 2021 14:35:55 +0000
Subject: [PATCH 7/7] Make organisations natively sortable

---
 app/main/views/organisations.py               |  2 +-
 app/models/__init__.py                        |  6 +++
 app/models/organisation.py                    |  9 ++++-
 app/models/service.py                         |  7 +---
 app/templates/views/organisations/index.html  |  2 +-
 .../views/organisations/test_organisations.py | 40 +++++++++++++------
 6 files changed, 45 insertions(+), 21 deletions(-)

diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py
index ac4a3e221..5c9eea7d0 100644
--- a/app/main/views/organisations.py
+++ b/app/main/views/organisations.py
@@ -116,7 +116,7 @@ def add_organisation_from_nhs_local_service(service_id):
 
     form = AddNHSLocalOrganisationForm(organisation_choices=[
         (organisation.id, organisation.name)
-        for organisation in AllOrganisations()
+        for organisation in sorted(AllOrganisations())
         if organisation.organisation_type == Organisation.TYPE_NHS_LOCAL
     ])
 
diff --git a/app/models/__init__.py b/app/models/__init__.py
index 0acd9a31a..2fd7bf723 100644
--- a/app/models/__init__.py
+++ b/app/models/__init__.py
@@ -60,3 +60,9 @@ class PaginatedModelList(ModelList):
         self.items = response[self.response_key]
         self.prev_page = response.get('links', {}).get('prev', None)
         self.next_page = response.get('links', {}).get('next', None)
+
+
+class SortByNameMixin():
+
+    def __lt__(self, other):
+        return self.name.lower() < other.name.lower()
diff --git a/app/models/organisation.py b/app/models/organisation.py
index bfb0fe9f2..cb9aa53ac 100644
--- a/app/models/organisation.py
+++ b/app/models/organisation.py
@@ -1,13 +1,18 @@
 from flask import abort
 from werkzeug.utils import cached_property
 
-from app.models import JSONModel, ModelList, SerialisedModelCollection
+from app.models import (
+    JSONModel,
+    ModelList,
+    SerialisedModelCollection,
+    SortByNameMixin,
+)
 from app.notify_client.email_branding_client import email_branding_client
 from app.notify_client.letter_branding_client import letter_branding_client
 from app.notify_client.organisations_api_client import organisations_client
 
 
-class Organisation(JSONModel):
+class Organisation(JSONModel, SortByNameMixin):
 
     TYPE_CENTRAL = 'central'
     TYPE_LOCAL = 'local'
diff --git a/app/models/service.py b/app/models/service.py
index ce77234c5..2a995c6d8 100644
--- a/app/models/service.py
+++ b/app/models/service.py
@@ -2,7 +2,7 @@ from flask import abort, current_app
 from notifications_utils.serialised_model import SerialisedModelCollection
 from werkzeug.utils import cached_property
 
-from app.models import JSONModel
+from app.models import JSONModel, SortByNameMixin
 from app.models.contact_list import ContactLists
 from app.models.job import (
     ImmediateJobs,
@@ -27,7 +27,7 @@ from app.notify_client.template_folder_api_client import (
 from app.utils import get_default_sms_sender
 
 
-class Service(JSONModel):
+class Service(JSONModel, SortByNameMixin):
 
     ALLOWED_PROPERTIES = {
         'active',
@@ -78,9 +78,6 @@ class Service(JSONModel):
     def from_id(cls, service_id):
         return cls(service_api_client.get_service(service_id)['data'])
 
-    def __lt__(self, other):
-        return self.name.lower() < other.name.lower()
-
     @property
     def permissions(self):
         return self._dict.get('permissions', self.TEMPLATE_TYPES)
diff --git a/app/templates/views/organisations/index.html b/app/templates/views/organisations/index.html
index c9a6fd8e3..71ae2752a 100644
--- a/app/templates/views/organisations/index.html
+++ b/app/templates/views/organisations/index.html
@@ -14,7 +14,7 @@
 
   <nav class="browse-list">
     <ul>
-      {% for org in organisations %}
+      {% for org in organisations|sort %}
         <li class="browse-list-item">
           <a href="{{ url_for('main.organisation_dashboard', org_id=org.id) }}" class="govuk-link govuk-link--no-visited-state">{{ org.name }}</a>
           {% if not org.active %}
diff --git a/tests/app/main/views/organisations/test_organisations.py b/tests/app/main/views/organisations/test_organisations.py
index ab0e5a82b..36b4514a4 100644
--- a/tests/app/main/views/organisations/test_organisations.py
+++ b/tests/app/main/views/organisations/test_organisations.py
@@ -20,9 +20,9 @@ def test_organisation_page_shows_all_organisations(
     mocker
 ):
     orgs = [
-        {'id': '1', 'name': 'Test 1', 'active': True, 'count_of_live_services': 0},
-        {'id': '2', 'name': 'Test 2', 'active': True, 'count_of_live_services': 1},
-        {'id': '3', 'name': 'Test 3', 'active': False, 'count_of_live_services': 2},
+        {'id': 'A3', 'name': 'Test 3', 'active': True, 'count_of_live_services': 0},
+        {'id': 'B1', 'name': 'Test 1', 'active': True, 'count_of_live_services': 1},
+        {'id': 'C2', 'name': 'Test 2', 'active': False, 'count_of_live_services': 2},
     ]
 
     get_organisations = mocker.patch(
@@ -39,15 +39,31 @@ def test_organisation_page_shows_all_organisations(
         page.select_one('h1').text
     ) == "Organisations"
 
-    expected_hints = ('0 live services', '1 live service', '2 live services')
-
-    for index, org in enumerate(orgs):
-        assert page.select('.browse-list-item a')[index].text == org['name']
-        if not org['active']:
-            assert page.select_one('.table-field-status-default,heading-medium').text == '- archived'
-        assert normalize_spaces(page.select('.browse-list-hint')[index].text) == (
-            expected_hints[index]
+    assert [
+        (
+            normalize_spaces(link.text),
+            normalize_spaces(hint.text),
+            link['href'],
+        ) for link, hint in zip(
+            page.select('.browse-list-item a'),
+            page.select('.browse-list-item .browse-list-hint'),
         )
+    ] == [
+        ('Test 1', '1 live service', url_for(
+            'main.organisation_dashboard', org_id='B1'
+        )),
+        ('Test 2', '2 live services', url_for(
+            'main.organisation_dashboard', org_id='C2'
+        )),
+        ('Test 3', '0 live services', url_for(
+            'main.organisation_dashboard', org_id='A3'
+        )),
+    ]
+
+    archived = page.select_one('.table-field-status-default.heading-medium')
+    assert normalize_spaces(archived.text) == '- archived'
+    assert normalize_spaces(archived.parent.text) == 'Test 2 - archived 2 live services'
+
     assert normalize_spaces(
         page.select_one('a.govuk-button--secondary').text
     ) == 'New organisation'
@@ -276,8 +292,8 @@ def test_nhs_local_can_create_own_organisations(
     mocker.patch(
         'app.models.organisation.AllOrganisations.client_method',
         return_value=[
-            organisation_json('t1', 'Trust 1', organisation_type='nhs_local'),
             organisation_json('t2', 'Trust 2', organisation_type='nhs_local'),
+            organisation_json('t1', 'Trust 1', organisation_type='nhs_local'),
             organisation_json('gp1', 'GP 1', organisation_type='nhs_gp'),
             organisation_json('c1', 'Central 1'),
         ],