From 047ca8a48cead9566897239cdaa1aeb3012d2432 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Thu, 2 Apr 2020 15:06:03 +0100
Subject: [PATCH] Fix unnecessary call to organisation API endpoint
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We’re caching the organisation name, but still talking to the API
to see if the organisation exists.

`Service().organisation_id` only goes to the JSON for the service.

`Service().organisation` makes a separate API call.

We only need the former to know if a service belongs to an organisation.
---
 app/models/service.py                     |  2 +-
 app/templates/views/service-settings.html |  2 +-
 tests/app/models/test_service.py          | 52 +++++++++++++++++++++++
 3 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/app/models/service.py b/app/models/service.py
index 241d325eb..f536fc64c 100644
--- a/app/models/service.py
+++ b/app/models/service.py
@@ -472,7 +472,7 @@ class Service(JSONModel):
 
     @property
     def organisation_name(self):
-        if not self.organisation:
+        if not self.organisation_id:
             return None
         return organisations_client.get_organisation_name(self.organisation_id)
 
diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html
index 767d611fe..d2cebc449 100644
--- a/app/templates/views/service-settings.html
+++ b/app/templates/views/service-settings.html
@@ -296,7 +296,7 @@
 
           {% call row() %}
             {{ text_field('Live')}}
-            {% if current_service.trial_mode and not current_service.organisation %}
+            {% if current_service.trial_mode and not current_service.organisation_id %}
               {{ text_field('No (organisation must be set first)') }}
               {{ text_field('') }}
             {% else %}
diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py
index 8c8a426fb..84f9d8377 100644
--- a/tests/app/models/test_service.py
+++ b/tests/app/models/test_service.py
@@ -1,5 +1,6 @@
 import uuid
 
+from app.models.organisation import Organisation
 from app.models.service import Service
 from app.models.user import User
 from tests import organisation_json
@@ -195,3 +196,54 @@ def test_organisation_type_when_service_and_its_org_both_have_an_org_type(mocker
     mocker.patch('app.organisations_client.get_organisation', return_value=org)
 
     assert service.organisation_type == 'local'
+
+
+def test_organisation_name_comes_from_cache(mocker, service_one):
+    mock_redis_get = mocker.patch(
+        'app.extensions.RedisClient.get',
+        return_value=b'"Borchester Council"',
+    )
+    mock_get_organisation = mocker.patch('app.organisations_client.get_organisation')
+    service = Service(service_one)
+    service._dict['organisation'] = ORGANISATION_ID
+
+    assert service.organisation_name == 'Borchester Council'
+    mock_redis_get.assert_called_once_with(f'organisation-{ORGANISATION_ID}-name')
+    assert mock_get_organisation.called is False
+
+
+def test_organisation_name_goes_into_cache(mocker, service_one):
+    mocker.patch(
+        'app.extensions.RedisClient.get',
+        return_value=None,
+    )
+    mock_redis_set = mocker.patch(
+        'app.extensions.RedisClient.set',
+    )
+    mocker.patch(
+        'app.organisations_client.get_organisation',
+        return_value=organisation_json(),
+    )
+    service = Service(service_one)
+    service._dict['organisation'] = ORGANISATION_ID
+
+    assert service.organisation_name == 'Test Organisation'
+    mock_redis_set.assert_called_once_with(
+        f'organisation-{ORGANISATION_ID}-name',
+        '"Test Organisation"',
+        ex=604800,
+    )
+
+
+def test_service_without_organisation_doesnt_need_org_api(mocker, service_one):
+    mock_redis_get = mocker.patch('app.extensions.RedisClient.get')
+    mock_get_organisation = mocker.patch('app.organisations_client.get_organisation')
+    service = Service(service_one)
+    service._dict['organisation'] = None
+
+    assert service.organisation_id is None
+    assert service.organisation_name is None
+    assert isinstance(service.organisation, Organisation)
+
+    assert mock_redis_get.called is False
+    assert mock_get_organisation.called is False