From 4dd245ca87a1549a92b99eceeb57abdb88f15266 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 12 Jun 2019 11:09:36 +0100 Subject: [PATCH 1/2] Remove deprecated fields from orgs and services MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Once the admin app has started using the new fields returned in https://github.com/alphagov/notifications-api/pull/2539 these fields won’t be needed any more. --- app/user/rest.py | 25 -------------------- tests/app/user/test_rest.py | 46 ------------------------------------- 2 files changed, 71 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index 444dac914..bede8d145 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -507,35 +507,10 @@ def get_orgs_and_services(user): { 'name': org.name, 'id': org.id, - 'services': [ - { - 'id': service.id, - 'name': service.name, - 'restricted': service.restricted, - } - for service in org.services - if service.active and service in user.services - ], 'count_of_live_services': len(org.live_services), } for org in user.organisations if org.active ], - 'services_without_organisations': [ - { - 'id': service.id, - 'name': service.name, - 'restricted': service.restricted, - } for service in user.services - if ( - service.active and - # include services that either aren't in an organisation, or are in an organisation, - # but not one that the user can see. - ( - not service.organisation or - service.organisation not in user.organisations - ) - ) - ], 'services': [ { 'id': service.id, diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 8a51d8968..801c9b6e3 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -837,41 +837,20 @@ def test_get_orgs_and_services_nests_services(admin_request, sample_user): 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, @@ -917,30 +896,15 @@ def test_get_orgs_and_services_only_returns_active(admin_request, sample_user): 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, @@ -983,25 +947,15 @@ def test_get_orgs_and_services_only_shows_users_orgs_and_services(admin_request, 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'] == [ From 0955403a3bcf830c4ddf33eab04563a4cf3f99bd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 13 Jun 2019 15:54:57 +0100 Subject: [PATCH 2/2] Slim down the /organisations response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the moment this response returns a list of service IDs for hundreds of organisations. The admin app doesn’t use this information, but having to wait for it to be serialized and sent across the network slows it down all the same. --- app/models.py | 19 ++++++++++++++++--- app/organisation/rest.py | 2 +- tests/app/db.py | 7 +++++-- tests/app/organisation/test_rest.py | 12 +++++++----- 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/app/models.py b/app/models.py index 320aa33de..a50a19868 100644 --- a/app/models.py +++ b/app/models.py @@ -376,6 +376,12 @@ class Organisation(db.Model): if service.active and not service.restricted ] + @property + def domain_list(self): + return [ + domain.domain for domain in self.domains + ] + def serialize(self): return { "id": str(self.id), @@ -391,13 +397,20 @@ class Organisation(db.Model): "agreement_signed_on_behalf_of_name": self.agreement_signed_on_behalf_of_name, "agreement_signed_on_behalf_of_email_address": self.agreement_signed_on_behalf_of_email_address, "agreement_signed_version": self.agreement_signed_version, - "domains": [ - domain.domain for domain in self.domains - ], + "domains": self.domain_list, "request_to_go_live_notes": self.request_to_go_live_notes, "count_of_live_services": len(self.live_services), } + def serialize_for_list(self): + return { + 'name': self.name, + 'id': str(self.id), + 'active': self.active, + 'count_of_live_services': len(self.live_services), + 'domains': self.domain_list, + } + class Service(db.Model, Versioned): __tablename__ = 'services' diff --git a/app/organisation/rest.py b/app/organisation/rest.py index fbfe09d22..6296e63dd 100644 --- a/app/organisation/rest.py +++ b/app/organisation/rest.py @@ -45,7 +45,7 @@ def handle_integrity_error(exc): @organisation_blueprint.route('', methods=['GET']) def get_organisations(): organisations = [ - org.serialize() for org in dao_get_organisations() + org.serialize_for_list() for org in dao_get_organisations() ] return jsonify(organisations) diff --git a/tests/app/db.py b/tests/app/db.py index bb388272b..418f963fa 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -620,15 +620,18 @@ def create_domain(domain, organisation_id): return domain -def create_organisation(name='test_org_1', active=True, organisation_type=None): +def create_organisation(name='test_org_1', active=True, organisation_type=None, domains=None): data = { 'name': name, 'active': active, - 'organisation_type': organisation_type + 'organisation_type': organisation_type, } organisation = Organisation(**data) dao_create_organisation(organisation) + for domain in domains or []: + create_domain(domain, organisation.id) + return organisation diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index d99670d7a..9d9a42b4c 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -16,7 +16,7 @@ from tests.app.db import ( def test_get_all_organisations(admin_request, notify_db_session): create_organisation(name='inactive org', active=False) - create_organisation(name='active org') + create_organisation(name='active org', domains=['example.com']) response = admin_request.get( 'organisation.get_organisations', @@ -27,9 +27,11 @@ def test_get_all_organisations(admin_request, notify_db_session): assert response[0]['name'] == 'active org' assert response[0]['active'] is True assert response[0]['count_of_live_services'] == 0 + assert response[0]['domains'] == ['example.com'] assert response[1]['name'] == 'inactive org' assert response[1]['active'] is False assert response[1]['count_of_live_services'] == 0 + assert response[1]['domains'] == [] def test_get_organisation_by_id(admin_request, notify_db_session): @@ -78,10 +80,10 @@ def test_get_organisation_by_id(admin_request, notify_db_session): def test_get_organisation_by_id_returns_domains(admin_request, notify_db_session): - org = create_organisation() - - create_domain('foo.gov.uk', org.id) - create_domain('bar.gov.uk', org.id) + org = create_organisation(domains=[ + 'foo.gov.uk', + 'bar.gov.uk', + ]) response = admin_request.get( 'organisation.get_organisation_by_id',