Merge pull request #4034 from alphagov/refactor-services-models

Refactor services and organisations models for users

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(

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 sorted(AllOrganisations())
         if organisation.organisation_type == Organisation.TYPE_NHS_LOCAL
     ])
 

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

app/models/organisation.py

@@ -1,13 +1,18 @@
 from flask import abort
 from werkzeug.utils import cached_property
 
-from app.models import JSONModel, ModelList
+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'
@@ -59,6 +64,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))
@@ -201,6 +209,9 @@ class Organisation(JSONModel):
         return organisations_client.get_services_and_usage(self.id, financial_year)
 
 
-class Organisations(ModelList):
-    client_method = organisations_client.get_organisations
+class Organisations(SerialisedModelCollection):
     model = Organisation
+
+
+class AllOrganisations(ModelList, Organisations):
+    client_method = organisations_client.get_organisations

app/models/service.py

@@ -1,7 +1,8 @@
 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,
@@ -26,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',
@@ -707,3 +708,7 @@ class Service(JSONModel):
     @property
     def contact_lists(self):
         return ContactLists(self.id)
+
+
+class Services(SerialisedModelCollection):
+    model = Service

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
@@ -290,16 +290,10 @@ 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([
-            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):
@@ -326,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):

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>

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>

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 %}