From b6fdb269e4bcb2c589f2e6bae1fd0118df96acf2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 12 Jun 2019 10:34:15 +0100 Subject: [PATCH 1/2] Return all required org and services info for user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin app now needs to know a few extra things about orgs and services in order to list them. At the moment it does this by making multiple API calls. This commit adds extra fields to the existing response. Once the admin app is using this fields we’ll be able to remove: - `reponse['services_without_organisations']` - `reponse['organisations']['services']` --- app/user/rest.py | 15 ++- tests/app/user/test_rest.py | 199 ++++++++++++++++++++++++------------ 2 files changed, 145 insertions(+), 69 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index 456008c30..872735de2 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -515,7 +515,11 @@ def get_orgs_and_services(user): } for service in org.services if service.active and service in user.services - ] + ], + 'count_of_live_services': len([ + service for service in org.services + if service.active and not service.restricted + ]), } for org in user.organisations if org.active ], @@ -534,5 +538,14 @@ def get_orgs_and_services(user): service.organisation not in user.organisations ) ) + ], + 'services': [ + { + 'id': service.id, + 'name': service.name, + 'restricted': service.restricted, + 'organisation': service.organisation.id if service.organisation else None, + } + for service in user.services if service.active ] } diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 35e3809ce..8a51d8968 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -835,38 +835,63 @@ def test_get_orgs_and_services_nests_services(admin_request, sample_user): resp = admin_request.get('user.get_organisations_and_services_for_user', user_id=sample_user.id) - assert resp == { - 'organisations': [ - { - 'name': org1.name, - 'id': str(org1.id), - 'services': [ - { - 'name': service1.name, - 'id': str(service1.id), - 'restricted': False, - }, - { - 'name': service2.name, - 'id': str(service2.id), - 'restricted': False, - } - ] - }, - { - 'name': org2.name, - 'id': str(org2.id), - 'services': [] - } - ], - 'services_without_organisations': [ - { - 'name': service3.name, - 'id': str(service3.id), - 'restricted': False, - } - ] + assert set(resp.keys()) == { + 'organisations', + 'services_without_organisations', + 'services', } + assert resp['organisations'] == [ + { + 'name': org1.name, + 'id': str(org1.id), + 'services': [ + { + 'name': service1.name, + 'id': str(service1.id), + 'restricted': False, + }, + { + 'name': service2.name, + 'id': str(service2.id), + 'restricted': False, + } + ], + 'count_of_live_services': 2, + }, + { + 'name': org2.name, + 'id': str(org2.id), + 'services': [], + 'count_of_live_services': 0, + }, + ] + assert resp['services_without_organisations'] == [ + { + 'name': service3.name, + 'id': str(service3.id), + 'restricted': False, + } + ] + assert resp['services'] == [ + { + 'name': service1.name, + 'id': str(service1.id), + 'restricted': False, + 'organisation': str(org1.id), + }, + { + 'name': service2.name, + 'id': str(service2.id), + 'restricted': False, + 'organisation': str(org1.id), + }, + { + 'name': service3.name, + 'id': str(service3.id), + 'restricted': False, + 'organisation': None, + }, + ] def test_get_orgs_and_services_only_returns_active(admin_request, sample_user): @@ -890,28 +915,52 @@ def test_get_orgs_and_services_only_returns_active(admin_request, sample_user): resp = admin_request.get('user.get_organisations_and_services_for_user', user_id=sample_user.id) - assert resp == { - 'organisations': [ - { - 'name': org1.name, - 'id': str(org1.id), - 'services': [ - { - 'name': service1.name, - 'id': str(service1.id), - 'restricted': False, - } - ] - } - ], - 'services_without_organisations': [ - { - 'name': service4.name, - 'id': str(service4.id), - 'restricted': False, - } - ] + assert set(resp.keys()) == { + 'organisations', + 'services_without_organisations', + 'services', } + assert resp['organisations'] == [ + { + 'name': org1.name, + 'id': str(org1.id), + 'services': [ + { + 'name': service1.name, + 'id': str(service1.id), + 'restricted': False, + } + ], + 'count_of_live_services': 1, + } + ] + assert resp['services_without_organisations'] == [ + { + 'name': service4.name, + 'id': str(service4.id), + 'restricted': False, + } + ] + assert resp['services'] == [ + { + 'name': service1.name, + 'id': str(service1.id), + 'restricted': False, + 'organisation': str(org1.id) + }, + { + 'name': service3.name, + 'id': str(service3.id), + 'restricted': False, + 'organisation': str(org2.id) + }, + { + 'name': service4.name, + 'id': str(service4.id), + 'restricted': False, + 'organisation': None, + }, + ] def test_get_orgs_and_services_only_shows_users_orgs_and_services(admin_request, sample_user): @@ -932,23 +981,37 @@ def test_get_orgs_and_services_only_shows_users_orgs_and_services(admin_request, resp = admin_request.get('user.get_organisations_and_services_for_user', user_id=sample_user.id) - assert resp == { - 'organisations': [ - { - 'name': org2.name, - 'id': str(org2.id), - 'services': [] - } - ], - # service1 belongs to org1, but the user doesn't know about org1 - 'services_without_organisations': [ - { - 'name': service1.name, - 'id': str(service1.id), - 'restricted': False, - } - ] + assert set(resp.keys()) == { + 'organisations', + 'services_without_organisations', + 'services', } + assert resp['organisations'] == [ + { + 'name': org2.name, + 'id': str(org2.id), + 'services': [], + 'count_of_live_services': 0, + } + ] + # service1 belongs to org1, but the user doesn't know about org1 + assert resp['services_without_organisations'] == [ + { + 'name': service1.name, + 'id': str(service1.id), + 'restricted': False, + } + ] + # 'services' always returns the org_id no matter whether the user + # belongs to that org or not + assert resp['services'] == [ + { + 'name': service1.name, + 'id': str(service1.id), + 'restricted': False, + 'organisation': str(org1.id), + } + ] def test_find_users_by_email_finds_user_by_partial_email(notify_db, client): From d974ab3b868846f47bc8d4749a58e19d3ce651ff Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 12 Jun 2019 13:15:25 +0100 Subject: [PATCH 2/2] Return count of live services on organisations too This makes it consistent, so the admin app can always rely on that property being available. --- app/models.py | 8 ++++++++ app/user/rest.py | 5 +---- tests/app/organisation/test_rest.py | 4 ++++ 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/app/models.py b/app/models.py index 0465214f4..cf3bc47a7 100644 --- a/app/models.py +++ b/app/models.py @@ -367,6 +367,13 @@ class Organisation(db.Model): nullable=True, ) + @property + def live_services(self): + return [ + service for service in self.services + if service.active and not service.restricted + ] + def serialize(self): return { "id": str(self.id), @@ -384,6 +391,7 @@ class Organisation(db.Model): domain.domain for domain in self.domains ], "request_to_go_live_notes": self.request_to_go_live_notes, + "count_of_live_services": len(self.live_services), } diff --git a/app/user/rest.py b/app/user/rest.py index 872735de2..444dac914 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -516,10 +516,7 @@ def get_orgs_and_services(user): for service in org.services if service.active and service in user.services ], - 'count_of_live_services': len([ - service for service in org.services - if service.active and not service.restricted - ]), + 'count_of_live_services': len(org.live_services), } for org in user.organisations if org.active ], diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index 4c231e50b..26a1155fc 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -26,8 +26,10 @@ def test_get_all_organisations(admin_request, notify_db_session): assert len(response) == 2 assert response[0]['name'] == 'active org' assert response[0]['active'] is True + assert response[0]['count_of_live_services'] == 0 assert response[1]['name'] == 'inactive org' assert response[1]['active'] is False + assert response[1]['count_of_live_services'] == 0 def test_get_organisation_by_id(admin_request, notify_db_session): @@ -53,6 +55,7 @@ def test_get_organisation_by_id(admin_request, notify_db_session): 'email_branding_id', 'domains', 'request_to_go_live_notes', + 'count_of_live_services', } assert response['id'] == str(org.id) assert response['name'] == 'test_org_1' @@ -66,6 +69,7 @@ def test_get_organisation_by_id(admin_request, notify_db_session): assert response['email_branding_id'] is None assert response['domains'] == [] assert response['request_to_go_live_notes'] is None + assert response['count_of_live_services'] == 0 def test_get_organisation_by_id_returns_domains(admin_request, notify_db_session):