Use new fields for getting orgs and services

Uses https://github.com/alphagov/notifications-api/pull/2539 to reduce
the number of API calls we make.

app/models/__init__.py

@@ -12,6 +12,12 @@ class JSONModel():
     def __bool__(self):
         return self._dict != {}
 
+    def __hash__(self):
+        return hash(self.id)
+
+    def __eq__(self, other):
+        return self.id == other.id
+
     def __getattr__(self, attr):
         if attr in self.ALLOWED_PROPERTIES:
             return self._dict[attr]

app/models/organisation.py

@@ -21,6 +21,7 @@ class Organisation(JSONModel):
         'agreement_signed_version',
         'domains',
         'request_to_go_live_notes',
+        'count_of_live_services',
     }
 
     @classmethod

app/models/service.py

@@ -402,6 +402,10 @@ class Service(JSONModel):
     def organisation(self):
         return Organisation.from_service(self.id)
 
+    @property
+    def organisation_id(self):
+        return self._dict['organisation']
+
     @cached_property
     def inbound_number(self):
         return inbound_number_client.get_inbound_sms_number_for_service(self.id)['data'].get('number', '')

app/models/user.py

@@ -1,5 +1,4 @@
 from collections.abc import Sequence
-from itertools import chain
 
 from flask import abort, current_app, request, session
 from flask_login import AnonymousUserMixin, UserMixin, login_user
@@ -247,29 +246,29 @@ 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):
-        return sorted(
+        from app.models.service import Service
-            self.services_with_organisation + self.services_without_organisations,
+        return self.sort_services([
-            key=lambda service: service.name.lower(),
+            Service(service) for service in self.orgs_and_services['services']
-        )
+        ])
 
     @property
     def services_with_organisation(self):
-        from app.models.service import Service
         return [
-            Service(service) for service in
+            service for service in self.services
-            next(chain(
+            if self.belongs_to_organisation(service.organisation_id)
-                org['services'] for org in self.orgs_and_services['organisations']
-            ), [])
         ]
 
     @property
     def services_without_organisations(self):
-        from app.models.service import Service
         return [
-            Service(service) for service in
+            service for service in self.services
-            self.orgs_and_services['services_without_organisations']
+            if not self.belongs_to_organisation(service.organisation_id)
         ]
 
     @property
@@ -290,12 +289,9 @@ class User(JSONModel, UserMixin):
 
     @property
     def live_services_not_belonging_to_users_organisations(self):
-        from app.models.service import Service
+        return self.sort_services(
-        return [
+            set(self.live_services).union(self.services_without_organisations)
-            Service(service)
+        )
-            for service in self.orgs_and_services['services_without_organisations']
-            if not service['restricted']
-        ]
 
     @property
     def organisations(self):

app/templates/views/choose-account.html

@@ -22,8 +22,8 @@
     <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 }}
+        {{ org.count_of_live_services }}
-        live service{% if org.live_services|length != 1 %}s{% endif %}
+        live service{% if org.count_of_live_services != 1 %}s{% endif %}
       </p>
     </li>
   {% endfor %}

tests/__init__.py

@@ -139,6 +139,7 @@ def service_json(
     organisation_type='central',
     prefix_sms=True,
     contact_link=None,
+    organisation_id=None,
 ):
     if users is None:
         users = []
@@ -173,6 +174,7 @@ def service_json(
         'volume_letter': 333333,
         'consent_to_research': True,
         'count_as_live': True,
+        'organisation': organisation_id,
     }
 
 
@@ -200,7 +202,6 @@ def organisation_json(
         'name': 'Test Organisation' if name is False else name,
         'active': active,
         'users': users,
-        'services': services,
         'created_at': created_at or str(datetime.utcnow()),
         'email_branding_id': email_branding_id,
         'letter_branding_id': letter_branding_id,
@@ -211,6 +212,7 @@ def organisation_json(
         'agreement_signed_by': None,
         'domains': domains or [],
         'request_to_go_live_notes': request_to_go_live_notes,
+        'count_of_live_services': len(services),
     }
 
 

tests/app/main/views/accounts/test_choose_accounts.py

@@ -1,42 +1,72 @@
+import uuid
+from itertools import repeat
+
 import pytest
 from bs4 import BeautifulSoup
 from flask import url_for
 
 from tests.conftest import (
     SERVICE_ONE_ID,
+    SERVICE_TWO_ID,
     normalize_spaces,
     service_one,
     service_two,
 )
 
+OS1, OS2, OS3, S1, S2, S3 = repeat(uuid.uuid4(), 6)
+
 SAMPLE_DATA = {
     'organisations': [
         {
             'name': 'org_1',
             'id': 'o1',
-            'services': [
-                {'name': 'org_service_1', 'id': 'os1', 'restricted': False},
-                {'name': 'org_service_2', 'id': 'os2', 'restricted': False},
-                {'name': 'org_service_3', 'id': 'os3', 'restricted': True},
-            ]
         },
         {
             'name': 'org_2',
             'id': 'o2',
-            'services': [
-                {'name': 'org_service_4', 'id': 'os4', 'restricted': False},
-            ]
         },
         {
             'name': 'org_3',
             'id': 'o3',
-            'services': []
         }
     ],
-    'services_without_organisations': [
+    'services': [
-        {'name': 'service_1', 'id': 's1', 'restricted': False},
+        {
-        {'name': 'service_2', 'id': 's2', 'restricted': False},
+            'name': 'org_service_1',
-        {'name': 'service_3', 'id': 's3', 'restricted': True},
+            'id': OS1,
+            'restricted': False,
+            'organisation': 'o1',
+        },
+        {
+            'name': 'org_service_2',
+            'id': OS2,
+            'restricted': False,
+            'organisation': 'o1',
+        },
+        {
+            'name': 'org_service_3',
+            'id': OS3,
+            'restricted': True,
+            'organisation': 'o1',
+        },
+        {
+            'name': 'service_1',
+            'id': S1,
+            'restricted': False,
+            'organisation': None,
+        },
+        {
+            'name': 'service_2',
+            'id': S2,
+            'restricted': False,
+            'organisation': None,
+        },
+        {
+            'name': 'service_3',
+            'id': S3,
+            'restricted': True,
+            'organisation': None,
+        },
     ]
 }
 
@@ -51,29 +81,29 @@ def mock_get_orgs_and_services(mocker):
 
 def test_choose_account_should_show_choose_accounts_page(
     client_request,
-    mock_get_orgs_and_services,
+    mock_get_non_empty_organisations_and_services_for_user,
     mock_get_organisation,
-    mock_get_organisation_services,
 ):
     resp = client_request.get('main.choose_account')
     page = resp.find('div', {'id': 'content'}).main
 
     assert normalize_spaces(page.h1.text) == 'Choose service'
     outer_list_items = page.select('nav ul')[0].select('li')
-    assert len(outer_list_items) == 5
+
+    assert len(outer_list_items) == 7
 
     # first org
     assert outer_list_items[0].a.text == 'Org 1'
     assert outer_list_items[0].a['href'] == url_for('.organisation_dashboard', org_id='o1')
     assert normalize_spaces(outer_list_items[0].select_one('.browse-list-hint').text) == (
-        '1 live service'
+        '0 live services'
     )
 
     # second org
     assert outer_list_items[1].a.text == 'Org 2'
     assert outer_list_items[1].a['href'] == url_for('.organisation_dashboard', org_id='o2')
     assert normalize_spaces(outer_list_items[1].select_one('.browse-list-hint').text) == (
-        '2 live services'
+        '0 live services'
     )
 
     # third org
@@ -84,18 +114,20 @@ def test_choose_account_should_show_choose_accounts_page(
     )
 
     # orphaned live services
-    assert outer_list_items[3].a.text == 'service_1'
+    assert outer_list_items[3].a.text == 'Service 1'
-    assert outer_list_items[3].a['href'] == url_for('.service_dashboard', service_id='s1')
+    assert outer_list_items[3].a['href'] == url_for('.service_dashboard', service_id=SERVICE_TWO_ID)
-    assert outer_list_items[4].a.text == 'service_2'
+    assert outer_list_items[4].a.text == 'service one'
-    assert outer_list_items[4].a['href'] == url_for('.service_dashboard', service_id='s2')
+    assert outer_list_items[4].a['href'] == url_for('.service_dashboard', service_id='12345')
 
     # orphaned trial services
     trial_services_list_items = page.select('nav ul')[1].select('li')
-    assert len(trial_services_list_items) == 2
+    assert len(trial_services_list_items) == 3
-    assert trial_services_list_items[0].a.text == 'org_service_3'
+    assert trial_services_list_items[0].a.text == 'service three'
-    assert trial_services_list_items[0].a['href'] == url_for('.service_dashboard', service_id='os3')
+    assert trial_services_list_items[0].a['href'] == url_for('.service_dashboard', service_id='abcde')
-    assert trial_services_list_items[1].a.text == 'service_3'
+    assert trial_services_list_items[1].a.text == 'service three'
-    assert trial_services_list_items[1].a['href'] == url_for('.service_dashboard', service_id='s3')
+    assert trial_services_list_items[1].a['href'] == url_for('.service_dashboard', service_id='abcde')
+
+    assert len(mock_get_organisation.call_args_list) == 21
 
 
 def test_choose_account_should_show_choose_accounts_page_if_no_services(
@@ -106,7 +138,7 @@ def test_choose_account_should_show_choose_accounts_page_if_no_services(
 ):
     mock_get_orgs_and_services.return_value = {
         'organisations': [],
-        'services_without_organisations': []
+        'services': []
     }
     resp = client_request.get('main.choose_account')
     page = resp.find('div', {'id': 'content'}).main

tests/app/main/views/accounts/test_show_accounts_or_dashboard.py

@@ -29,7 +29,7 @@ def user_with_orgs_and_services(num_orgs, num_services, platform_admin=False):
 def test_show_accounts_or_dashboard_redirects_to_choose_account_or_service_dashboard(
     client,
     mocker,
-    mock_get_non_empty_organisations_and_services_for_user,
+    mock_get_organisations_and_services_for_user,
     num_orgs,
     num_services,
     endpoint,
@@ -78,7 +78,7 @@ def test_show_accounts_or_dashboard_redirects_if_org_in_session(client, mocker):
 def test_show_accounts_or_dashboard_doesnt_redirect_to_service_dashboard_if_user_not_part_of_service_in_session(
     client,
     mocker,
-    mock_get_non_empty_organisations_and_services_for_user,
+    mock_get_organisations_and_services_for_user,
     mock_get_service
 ):
     client.login(user_with_orgs_and_services(num_orgs=1, num_services=1), mocker=mocker)
@@ -95,7 +95,7 @@ def test_show_accounts_or_dashboard_doesnt_redirect_to_service_dashboard_if_user
 def test_show_accounts_or_dashboard_doesnt_redirect_to_org_dashboard_if_user_not_part_of_org_in_session(
     client,
     mocker,
-    mock_get_non_empty_organisations_and_services_for_user,
+    mock_get_organisations_and_services_for_user,
 ):
     client.login(user_with_orgs_and_services(num_orgs=1, num_services=1), mocker=mocker)
     with client.session_transaction() as session:

tests/app/main/views/test_find_users.py

@@ -100,7 +100,7 @@ def test_user_information_page_shows_information_about_user(
 
     mocker.patch(
         'app.user_api_client.get_organisations_and_services_for_user',
-        return_value={'organisations': [], 'services_without_organisations': [
+        return_value={'organisations': [], 'services': [
             {"id": 1, "name": "Fresh Orchard Juice", "restricted": True},
             {"id": 2, "name": "Nature Therapy", "restricted": False},
         ]},
@@ -138,7 +138,7 @@ def test_user_information_page_displays_if_there_are_failed_login_attempts(
 
     mocker.patch(
         'app.user_api_client.get_organisations_and_services_for_user',
-        return_value={'organisations': [], 'services_without_organisations': [
+        return_value={'organisations': [], 'services': [
             {"id": 1, "name": "Fresh Orchard Juice", "restricted": True},
             {"id": 2, "name": "Nature Therapy", "restricted": True},
         ]},

tests/app/main/views/test_templates.py

@@ -840,7 +840,7 @@ def test_choose_a_template_to_copy(
     client_request,
     mock_get_service_templates,
     mock_get_template_folders,
-    mock_get_non_empty_organisations_and_services_for_user,
+    mock_get_just_services_for_user,
 ):
     page = client_request.get(
         'main.choose_template_to_copy',
@@ -850,62 +850,6 @@ def test_choose_a_template_to_copy(
     assert page.select('.folder-heading') == []
 
     expected = [
-        (
-            'Org 1 service 1 '
-            '6 templates'
-        ),
-        (
-            'Org 1 service 1 / sms_template_one '
-            'Text message template'
-        ),
-        (
-            'Org 1 service 1 / sms_template_two '
-            'Text message template'
-        ),
-        (
-            'Org 1 service 1 / email_template_one '
-            'Email template'
-        ),
-        (
-            'Org 1 service 1 / email_template_two '
-            'Email template'
-        ),
-        (
-            'Org 1 service 1 / letter_template_one '
-            'Letter template'
-        ),
-        (
-            'Org 1 service 1 / letter_template_two '
-            'Letter template'
-        ),
-        (
-            'Org 1 service 2 '
-            '6 templates'
-        ),
-        (
-            'Org 1 service 2 / sms_template_one '
-            'Text message template'
-        ),
-        (
-            'Org 1 service 2 / sms_template_two '
-            'Text message template'
-        ),
-        (
-            'Org 1 service 2 / email_template_one '
-            'Email template'
-        ),
-        (
-            'Org 1 service 2 / email_template_two '
-            'Email template'
-        ),
-        (
-            'Org 1 service 2 / letter_template_one '
-            'Letter template'
-        ),
-        (
-            'Org 1 service 2 / letter_template_two '
-            'Letter template'
-        ),
         (
             'Service 1 '
             '6 templates'

tests/conftest.py

@@ -3207,27 +3207,28 @@ def mock_update_service_organisation(mocker):
     )
 
 
+def _get_organisation_services(organisation_id):
+    if organisation_id == 'o1':
+        return [
+            service_json('12345', 'service one', restricted=False),
+            service_json('67890', 'service two'),
+            service_json('abcde', 'service three'),
+        ]
+    if organisation_id == 'o2':
+        return [
+            service_json('12345', 'service one', restricted=False),
+            service_json('67890', 'service two', restricted=False),
+            service_json('abcde', 'service three'),
+        ]
+    return [
+        service_json('12345', 'service one'),
+        service_json('67890', 'service two'),
+        service_json(SERVICE_ONE_ID, 'service one', [api_user_active(fake_uuid())['id']])
+    ]
+
+
 @pytest.fixture(scope='function')
 def mock_get_organisation_services(mocker, api_user_active):
-    def _get_organisation_services(organisation_id):
-        if organisation_id == 'o1':
-            return [
-                service_json('12345', 'service one', restricted=False),
-                service_json('67890', 'service two'),
-                service_json('abcde', 'service three'),
-            ]
-        if organisation_id == 'o2':
-            return [
-                service_json('12345', 'service one', restricted=False),
-                service_json('67890', 'service two', restricted=False),
-                service_json('abcde', 'service three'),
-            ]
-        return [
-            service_json('12345', 'service one'),
-            service_json('67890', 'service two'),
-            service_json(SERVICE_ONE_ID, 'service one', [api_user_active['id']])
-        ]
-
     return mocker.patch(
         'app.organisations_client.get_organisation_services',
         side_effect=_get_organisation_services
@@ -3346,7 +3347,7 @@ def mock_get_organisations_and_services_for_user(mocker, organisation_one, api_u
     def _get_orgs_and_services(user_id):
         return {
             'organisations': [],
-            'services_without_organisations': []
+            'services': []
         }
 
     return mocker.patch(
@@ -3358,20 +3359,61 @@ def mock_get_organisations_and_services_for_user(mocker, organisation_one, api_u
 @pytest.fixture
 def mock_get_non_empty_organisations_and_services_for_user(mocker, organisation_one, api_user_active):
 
-    def _make_services(name):
+    def _make_services(name, trial_mode=False):
         return [{
             'name': '{} {}'.format(name, i),
             'id': SERVICE_TWO_ID,
-            'restricted': False,
+            'restricted': trial_mode,
+            'organisation': None,
         } for i in range(1, 3)]
 
     def _get_orgs_and_services(user_id):
         return {
             'organisations': [
-                {'name': 'Org 1', 'services': _make_services('Org 1 service')},
+                {
-                {'name': 'Org 2', 'services': _make_services('Org 2 service')},
+                    'name': 'Org 1',
+                    'id': 'o1',
+                    'count_of_live_services': 1,
+                },
+                {
+                    'name': 'Org 2',
+                    'id': 'o2',
+                    'count_of_live_services': 2,
+                },
+                {
+                    'name': 'Org 3',
+                    'id': 'o3',
+                    'count_of_live_services': 0,
+                },
             ],
-            'services_without_organisations': _make_services('Service')
+            'services': (
+                _get_organisation_services('o1')
+                + _get_organisation_services('o2')
+                + _make_services('Service')
+            )
+        }
+
+    return mocker.patch(
+        'app.user_api_client.get_organisations_and_services_for_user',
+        side_effect=_get_orgs_and_services
+    )
+
+
+@pytest.fixture
+def mock_get_just_services_for_user(mocker, organisation_one, api_user_active):
+
+    def _make_services(name, trial_mode=False):
+        return [{
+            'name': '{} {}'.format(name, i + 1),
+            'id': id,
+            'restricted': trial_mode,
+            'organisation': None,
+        } for i, id in enumerate([SERVICE_TWO_ID, SERVICE_ONE_ID])]
+
+    def _get_orgs_and_services(user_id):
+        return {
+            'organisations': [],
+            'services': _make_services('Service'),
         }
 
     return mocker.patch(
@@ -3386,7 +3428,7 @@ def mock_get_empty_organisations_and_one_service_for_user(mocker, organisation_o
     def _get_orgs_and_services(user_id):
         return {
             'organisations': [],
-            'services_without_organisations': [{
+            'services': [{
                 'name': 'Only service',
                 'id': SERVICE_TWO_ID,
             }]