From 0aea038d511577c0cece351380a9af7ea8f52ceb Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 12 Jun 2019 12:09:26 +0100 Subject: [PATCH 1/4] 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 | 6 ++ app/models/organisation.py | 1 + app/models/service.py | 4 + app/models/user.py | 34 +++---- app/templates/views/choose-account.html | 4 +- tests/__init__.py | 4 +- .../views/accounts/test_choose_accounts.py | 88 +++++++++++------ .../test_show_accounts_or_dashboard.py | 6 +- tests/app/main/views/test_find_users.py | 4 +- tests/app/main/views/test_templates.py | 58 +----------- tests/conftest.py | 94 ++++++++++++++----- 11 files changed, 165 insertions(+), 138 deletions(-) diff --git a/app/models/__init__.py b/app/models/__init__.py index 29cc7ea63..520918223 100644 --- a/app/models/__init__.py +++ b/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] diff --git a/app/models/organisation.py b/app/models/organisation.py index 7f8a78116..1fee293b2 100644 --- a/app/models/organisation.py +++ b/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 diff --git a/app/models/service.py b/app/models/service.py index 32cea9f68..c606f907f 100644 --- a/app/models/service.py +++ b/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', '') diff --git a/app/models/user.py b/app/models/user.py index 210fdcb3e..6119f5391 100644 --- a/app/models/user.py +++ b/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( - self.services_with_organisation + self.services_without_organisations, - key=lambda service: service.name.lower(), - ) + from app.models.service import Service + return self.sort_services([ + 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 - next(chain( - org['services'] for org in self.orgs_and_services['organisations'] - ), []) + service for service in self.services + if self.belongs_to_organisation(service.organisation_id) ] @property def services_without_organisations(self): - from app.models.service import Service return [ - Service(service) for service in - self.orgs_and_services['services_without_organisations'] + service for service in self.services + 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 [ - Service(service) - for service in self.orgs_and_services['services_without_organisations'] - if not service['restricted'] - ] + return self.sort_services( + set(self.live_services).union(self.services_without_organisations) + ) @property def organisations(self): diff --git a/app/templates/views/choose-account.html b/app/templates/views/choose-account.html index d60f43156..9457f520e 100644 --- a/app/templates/views/choose-account.html +++ b/app/templates/views/choose-account.html @@ -22,8 +22,8 @@
  • {{ org.name }}

    - {{ org.live_services|length }} - live service{% if org.live_services|length != 1 %}s{% endif %} + {{ org.count_of_live_services }} + live service{% if org.count_of_live_services != 1 %}s{% endif %}

    • {% endfor %} diff --git a/tests/__init__.py b/tests/__init__.py index 0fe944c57..777ccb7a6 100644 --- a/tests/__init__.py +++ b/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), } diff --git a/tests/app/main/views/accounts/test_choose_accounts.py b/tests/app/main/views/accounts/test_choose_accounts.py index ead1a283c..b8438975a 100644 --- a/tests/app/main/views/accounts/test_choose_accounts.py +++ b/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': [ - {'name': 'service_1', 'id': 's1', 'restricted': False}, - {'name': 'service_2', 'id': 's2', 'restricted': False}, - {'name': 'service_3', 'id': 's3', 'restricted': True}, + 'services': [ + { + 'name': 'org_service_1', + '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['href'] == url_for('.service_dashboard', service_id='s1') - assert outer_list_items[4].a.text == 'service_2' - assert outer_list_items[4].a['href'] == url_for('.service_dashboard', service_id='s2') + assert outer_list_items[3].a.text == 'Service 1' + assert outer_list_items[3].a['href'] == url_for('.service_dashboard', service_id=SERVICE_TWO_ID) + assert outer_list_items[4].a.text == 'service one' + 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 trial_services_list_items[0].a.text == 'org_service_3' - assert trial_services_list_items[0].a['href'] == url_for('.service_dashboard', service_id='os3') - assert trial_services_list_items[1].a.text == 'service_3' - assert trial_services_list_items[1].a['href'] == url_for('.service_dashboard', service_id='s3') + assert len(trial_services_list_items) == 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='abcde') + assert trial_services_list_items[1].a.text == 'service three' + 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 diff --git a/tests/app/main/views/accounts/test_show_accounts_or_dashboard.py b/tests/app/main/views/accounts/test_show_accounts_or_dashboard.py index d5c5a3edf..ba268dbef 100644 --- a/tests/app/main/views/accounts/test_show_accounts_or_dashboard.py +++ b/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: diff --git a/tests/app/main/views/test_find_users.py b/tests/app/main/views/test_find_users.py index c5de5b594..345c10bef 100644 --- a/tests/app/main/views/test_find_users.py +++ b/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}, ]}, diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 42cfdc67f..4dafca534 100644 --- a/tests/app/main/views/test_templates.py +++ b/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' diff --git a/tests/conftest.py b/tests/conftest.py index 1e8b8fa68..d087d02c7 100644 --- a/tests/conftest.py +++ b/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, }] From 913095fe607ada523d7ce031d376493462777fcd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 12 Jun 2019 13:40:57 +0100 Subject: [PATCH 2/4] Wrap a model around list of all organisations --- app/main/views/organisations.py | 5 ++-- app/models/__init__.py | 28 +++++++++++++++++++ app/models/organisation.py | 7 ++++- app/models/user.py | 19 +++---------- app/templates/views/organisations/index.html | 4 +-- .../views/organisations/test_organisation.py | 2 +- 6 files changed, 43 insertions(+), 22 deletions(-) diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index dcf81dd62..44d4c068d 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -31,6 +31,7 @@ from app.main.forms import ( SetLetterBranding, ) from app.main.views.service_settings import get_branding_as_value_and_label +from app.models.organisation import Organisations from app.models.user import InvitedOrgUser, User from app.utils import user_has_permissions, user_is_platform_admin @@ -39,11 +40,9 @@ from app.utils import user_has_permissions, user_is_platform_admin @login_required @user_is_platform_admin def organisations(): - orgs = organisations_client.get_organisations() - return render_template( 'views/organisations/index.html', - organisations=orgs, + organisations=Organisations(), search_form=SearchByNameForm(), ) diff --git a/app/models/__init__.py b/app/models/__init__.py index 520918223..7675d4a81 100644 --- a/app/models/__init__.py +++ b/app/models/__init__.py @@ -1,3 +1,6 @@ +from abc import ABC, abstractmethod +from collections.abc import Sequence + from flask import abort @@ -33,5 +36,30 @@ class JSONModel(): abort(404) +class ModelList(ABC, Sequence): + + @property + @abstractmethod + def client(): + pass + + @property + @abstractmethod + def model(): + pass + + def __init__(self): + self.items = self.client() + + def __getitem__(self, index): + return self.model(self.items[index]) + + def __len__(self): + return len(self.items) + + def __add__(self, other): + return list(self) + list(other) + + class InviteTokenError(Exception): pass diff --git a/app/models/organisation.py b/app/models/organisation.py index 1fee293b2..b0c10f863 100644 --- a/app/models/organisation.py +++ b/app/models/organisation.py @@ -1,7 +1,7 @@ from flask import Markup, abort from werkzeug.utils import cached_property -from app.models import JSONModel +from app.models import JSONModel, ModelList from app.notify_client.organisations_api_client import organisations_client @@ -155,3 +155,8 @@ class Organisation(JSONModel): self.invited_users + self.active_users, key=lambda user: user.email_address.lower(), ) + + +class Organisations(ModelList): + client = organisations_client.get_organisations + model = Organisation diff --git a/app/models/user.py b/app/models/user.py index 6119f5391..9b1be251a 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -1,11 +1,9 @@ -from collections.abc import Sequence - from flask import abort, current_app, request, session from flask_login import AnonymousUserMixin, UserMixin, login_user from notifications_python_client.errors import HTTPError from werkzeug.utils import cached_property -from app.models import JSONModel +from app.models import JSONModel, ModelList from app.models.organisation import Organisation from app.models.roles_and_permissions import ( all_permissions, @@ -590,22 +588,13 @@ class AnonymousUser(AnonymousUserMixin): return Organisation(None) -class Users(Sequence): +class Users(ModelList): client = user_api_client.get_users_for_service model = User def __init__(self, service_id): - self.users = self.client(service_id) - - def __getitem__(self, index): - return self.model(self.users[index]) - - def __len__(self): - return len(self.users) - - def __add__(self, other): - return list(self) + list(other) + self.items = self.client(service_id) class OrganisationUsers(Users): @@ -618,7 +607,7 @@ class InvitedUsers(Users): model = InvitedUser def __init__(self, service_id): - self.users = [ + self.items = [ user for user in self.client(service_id) if user['status'] != 'accepted' ] diff --git a/app/templates/views/organisations/index.html b/app/templates/views/organisations/index.html index e52677366..00a228459 100644 --- a/app/templates/views/organisations/index.html +++ b/app/templates/views/organisations/index.html @@ -32,8 +32,8 @@