From e04b2b5631c3af3f3c725eeddea142bf968fe245 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Oct 2018 15:58:44 +0100 Subject: [PATCH] Split models to prevent circular imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit is the first step to disentangling the models from the API clients. With the models in the same folder as the API clients it makes it hard to import the API clients within the model without getting a circular import. After this commit the user API clients still has this problem, but at least the service API client doesn’t. --- app/__init__.py | 4 +- app/main/forms.py | 2 +- app/main/views/add_service.py | 2 +- app/main/views/manage_users.py | 2 +- app/models/service.py | 157 ++++++++++++++++++ .../models.py => models/user.py} | 157 ------------------ app/notify_client/invite_api_client.py | 4 +- app/notify_client/org_invite_api_client.py | 2 +- app/notify_client/user_api_client.py | 4 +- tests/__init__.py | 4 +- tests/app/main/test_permissions.py | 2 +- .../test_show_accounts_or_dashboard.py | 2 +- .../test_organisation_invites.py | 2 +- tests/app/main/views/test_accept_invite.py | 2 +- tests/app/main/views/test_activity.py | 2 +- tests/app/main/views/test_manage_users.py | 2 +- tests/app/main/views/test_register.py | 2 +- tests/app/main/views/test_service_settings.py | 16 +- .../test_notify_admin_api_client.py | 2 +- tests/app/notify_client/test_user_client.py | 6 +- tests/conftest.py | 2 +- 21 files changed, 188 insertions(+), 190 deletions(-) create mode 100644 app/models/service.py rename app/{notify_client/models.py => models/user.py} (68%) diff --git a/app/__init__.py b/app/__init__.py index 792142baf..f39d4eaae 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -41,7 +41,8 @@ from werkzeug.local import LocalProxy from app import proxy_fix from app.config import configs from app.asset_fingerprinter import asset_fingerprinter -from app.notify_client.models import Service +from app.models.service import Service +from app.models.user import AnonymousUser from app.navigation import ( CaseworkNavigation, HeaderNavigation, @@ -59,7 +60,6 @@ from app.notify_client.user_api_client import user_api_client from app.notify_client.events_api_client import events_api_client from app.notify_client.provider_client import provider_client from app.notify_client.email_branding_client import email_branding_client -from app.notify_client.models import AnonymousUser from app.notify_client.organisations_api_client import organisations_client from app.notify_client.org_invite_api_client import org_invite_api_client from app.notify_client.letter_jobs_client import letter_jobs_client diff --git a/app/main/forms.py b/app/main/forms.py index 91df7bc59..643fdcd31 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -43,7 +43,7 @@ from app.main.validators import ( ValidEmail, ValidGovEmail, ) -from app.notify_client.models import permissions, roles +from app.models.user import permissions, roles from app.utils import AgreementInfo, guess_name_from_email_address diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index e311a70e6..114f43437 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -12,7 +12,7 @@ from app import ( ) from app.main import main from app.main.forms import CreateServiceForm -from app.notify_client.models import InvitedUser +from app.models.user import InvitedUser from app.utils import AgreementInfo, email_safe, is_gov_user diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 90b98450e..4c0642ead 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -10,7 +10,7 @@ from app import ( ) from app.main import main from app.main.forms import InviteUserForm, PermissionsForm, SearchUsersForm -from app.notify_client.models import permissions +from app.models.user import permissions from app.utils import user_has_permissions diff --git a/app/models/service.py b/app/models/service.py new file mode 100644 index 000000000..9d59f2e9a --- /dev/null +++ b/app/models/service.py @@ -0,0 +1,157 @@ +from werkzeug.utils import cached_property + +from app.utils import get_default_sms_sender + + +class Service(): + + ALLOWED_PROPERTIES = { + 'active', + 'contact_link', + 'dvla_organisation', + 'email_branding', + 'email_from', + 'id', + 'inbound_api', + 'letter_contact_block', + 'letter_logo_filename', + 'message_limit', + 'name', + 'organisation_type', + 'permissions', + 'postage', + 'prefix_sms', + 'research_mode', + 'service_callback_api', + } + + def __init__(self, _dict): + # in the case of a bad request current service may be `None` + self._dict = _dict or {} + if 'permissions' not in self._dict: + self.permissions = {'email', 'sms', 'letter'} + + def __bool__(self): + return self._dict != {} + + def __getattr__(self, attr): + if attr in self.ALLOWED_PROPERTIES: + return self._dict[attr] + raise AttributeError('`{}` is not a service attribute'.format(attr)) + + def __getitem__(self, attr): + raise NotImplementedError( + 'Use current_service.{} instead of current_service[\'{}\']'.format(attr, attr) + ) + + def get(self, attr, default=None): + try: + return self._dict[attr] + except KeyError: + return default + + @property + def trial_mode(self): + return self._dict['restricted'] + + def has_permission(self, permission): + return permission in self.permissions + + @cached_property + def has_jobs(self): + # Can’t import at top-level because app isn’t yet initialised + from app import job_api_client + return job_api_client.has_jobs(self.id) + + @cached_property + def has_team_members(self): + from app import user_api_client + return user_api_client.get_count_of_users_with_permission( + self.id, 'manage_service' + ) > 1 + + @cached_property + def templates(self): + + from app import service_api_client + + templates = service_api_client.get_service_templates(self.id)['data'] + + return [ + template for template in templates + if template['template_type'] in self.available_template_types + ] + + def templates_by_type(self, template_type): + return [ + template for template in self.templates + if template_type in {'all', template['template_type']} + ] + + @property + def available_template_types(self): + return [ + channel for channel in ('email', 'sms', 'letter') + if self.has_permission(channel) + ] + + @property + def has_templates(self): + return len(self.templates) > 0 + + @property + def has_multiple_template_types(self): + return len({ + template['template_type'] for template in self.templates + }) > 1 + + @property + def has_email_templates(self): + return len(self.templates_by_type('email')) > 0 + + @property + def has_sms_templates(self): + return len(self.templates_by_type('sms')) > 0 + + @cached_property + def has_email_reply_to_address(self): + from app import service_api_client + return bool(service_api_client.get_reply_to_email_addresses( + self.id + )) + + @property + def needs_to_add_email_reply_to_address(self): + return self.has_email_templates and not self.has_email_reply_to_address + + @property + def shouldnt_use_govuk_as_sms_sender(self): + return self.organisation_type in {'local', 'nhs'} + + @cached_property + def sms_sender_is_govuk(self): + from app import service_api_client + return get_default_sms_sender( + service_api_client.get_sms_senders(self.id) + ) in {'GOVUK', 'None'} + + @property + def needs_to_change_sms_sender(self): + return all(( + self.has_sms_templates, + self.shouldnt_use_govuk_as_sms_sender, + self.sms_sender_is_govuk, + )) + + @property + def go_live_checklist_completed(self): + return all(( + self.has_team_members, + self.has_templates, + not self.needs_to_add_email_reply_to_address, + not self.needs_to_change_sms_sender, + )) + + @property + def go_live_checklist_completed_as_yes_no(self): + return 'Yes' if self.go_live_checklist_completed else 'No' diff --git a/app/notify_client/models.py b/app/models/user.py similarity index 68% rename from app/notify_client/models.py rename to app/models/user.py index cc520e124..201743356 100644 --- a/app/notify_client/models.py +++ b/app/models/user.py @@ -2,9 +2,6 @@ from itertools import chain from flask import request, session from flask_login import AnonymousUserMixin, UserMixin -from werkzeug.utils import cached_property - -from app.utils import get_default_sms_sender roles = { 'send_messages': ['send_texts', 'send_emails', 'send_letters'], @@ -267,157 +264,3 @@ class AnonymousUser(AnonymousUserMixin): # set the anonymous user so that if a new browser hits us we don't error http://stackoverflow.com/a/19275188 def logged_in_elsewhere(self): return False - - -class Service(): - - ALLOWED_PROPERTIES = { - 'active', - 'contact_link', - 'dvla_organisation', - 'email_branding', - 'email_from', - 'id', - 'inbound_api', - 'letter_contact_block', - 'letter_logo_filename', - 'message_limit', - 'name', - 'organisation_type', - 'permissions', - 'postage', - 'prefix_sms', - 'research_mode', - 'service_callback_api', - } - - def __init__(self, _dict): - # in the case of a bad request current service may be `None` - self._dict = _dict or {} - if 'permissions' not in self._dict: - self.permissions = {'email', 'sms', 'letter'} - - def __bool__(self): - return self._dict != {} - - def __getattr__(self, attr): - if attr in self.ALLOWED_PROPERTIES: - return self._dict[attr] - raise AttributeError('`{}` is not a service attribute'.format(attr)) - - def __getitem__(self, attr): - raise NotImplementedError( - 'Use current_service.{} instead of current_service[\'{}\']'.format(attr, attr) - ) - - def get(self, attr, default=None): - try: - return self._dict[attr] - except KeyError: - return default - - @property - def trial_mode(self): - return self._dict['restricted'] - - def has_permission(self, permission): - return permission in self.permissions - - @cached_property - def has_jobs(self): - # Can’t import at top-level because app isn’t yet initialised - from app import job_api_client - return job_api_client.has_jobs(self.id) - - @cached_property - def has_team_members(self): - from app import user_api_client - return user_api_client.get_count_of_users_with_permission( - self.id, 'manage_service' - ) > 1 - - @cached_property - def templates(self): - - from app import service_api_client - - templates = service_api_client.get_service_templates(self.id)['data'] - - return [ - template for template in templates - if template['template_type'] in self.available_template_types - ] - - def templates_by_type(self, template_type): - return [ - template for template in self.templates - if template_type in {'all', template['template_type']} - ] - - @property - def available_template_types(self): - return [ - channel for channel in ('email', 'sms', 'letter') - if self.has_permission(channel) - ] - - @property - def has_templates(self): - return len(self.templates) > 0 - - @property - def has_multiple_template_types(self): - return len({ - template['template_type'] for template in self.templates - }) > 1 - - @property - def has_email_templates(self): - return len(self.templates_by_type('email')) > 0 - - @property - def has_sms_templates(self): - return len(self.templates_by_type('sms')) > 0 - - @cached_property - def has_email_reply_to_address(self): - from app import service_api_client - return bool(service_api_client.get_reply_to_email_addresses( - self.id - )) - - @property - def needs_to_add_email_reply_to_address(self): - return self.has_email_templates and not self.has_email_reply_to_address - - @property - def shouldnt_use_govuk_as_sms_sender(self): - return self.organisation_type in {'local', 'nhs'} - - @cached_property - def sms_sender_is_govuk(self): - from app import service_api_client - return get_default_sms_sender( - service_api_client.get_sms_senders(self.id) - ) in {'GOVUK', 'None'} - - @property - def needs_to_change_sms_sender(self): - return all(( - self.has_sms_templates, - self.shouldnt_use_govuk_as_sms_sender, - self.sms_sender_is_govuk, - )) - - @property - def go_live_checklist_completed(self): - return all(( - self.has_team_members, - self.has_templates, - not self.needs_to_add_email_reply_to_address, - not self.needs_to_change_sms_sender, - )) - - @property - def go_live_checklist_completed_as_yes_no(self): - return 'Yes' if self.go_live_checklist_completed else 'No' diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index f9a9c0b6c..9ea677157 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -1,8 +1,8 @@ -from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache -from app.notify_client.models import ( +from app.models.user import ( InvitedUser, translate_permissions_from_admin_roles_to_db, ) +from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache class InviteApiClient(NotifyAdminAPIClient): diff --git a/app/notify_client/org_invite_api_client.py b/app/notify_client/org_invite_api_client.py index c3fa632c1..f80b20ca3 100644 --- a/app/notify_client/org_invite_api_client.py +++ b/app/notify_client/org_invite_api_client.py @@ -1,5 +1,5 @@ +from app.models.user import InvitedOrgUser from app.notify_client import NotifyAdminAPIClient, _attach_current_user -from app.notify_client.models import InvitedOrgUser class OrgInviteApiClient(NotifyAdminAPIClient): diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 6f1daf6b8..78a694c88 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -2,12 +2,12 @@ from itertools import chain from notifications_python_client.errors import HTTPError -from app.notify_client import NotifyAdminAPIClient, cache -from app.notify_client.models import ( +from app.models.user import ( User, roles, translate_permissions_from_admin_roles_to_db, ) +from app.notify_client import NotifyAdminAPIClient, cache ALLOWED_ATTRIBUTES = { 'name', diff --git a/tests/__init__.py b/tests/__init__.py index 6a50ffcd6..f12e111b8 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -6,9 +6,7 @@ from datetime import datetime, timedelta, timezone from flask.testing import FlaskClient from flask import url_for from flask_login import login_user -from app.notify_client.models import ( - InvitedOrgUser, -) +from app.models.user import InvitedOrgUser class TestClient(FlaskClient): diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index 08ac18bd5..70b013df7 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -3,7 +3,7 @@ from flask import request from werkzeug.exceptions import Forbidden, Unauthorized from app.main.views.index import index -from app.notify_client.models import ( +from app.models.user import ( translate_permissions_from_admin_roles_to_db, translate_permissions_from_db_to_admin_roles, ) 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 6a0582be7..5eb5117b6 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 @@ -1,7 +1,7 @@ import pytest from flask import url_for -from app.notify_client.models import User +from app.models.user import User from tests import user_json diff --git a/tests/app/main/views/organisations/test_organisation_invites.py b/tests/app/main/views/organisations/test_organisation_invites.py index 10f5a7647..8960acbf2 100644 --- a/tests/app/main/views/organisations/test_organisation_invites.py +++ b/tests/app/main/views/organisations/test_organisation_invites.py @@ -6,7 +6,7 @@ from bs4 import BeautifulSoup from flask import url_for from notifications_python_client.errors import HTTPError -from app.notify_client.models import InvitedOrgUser +from app.models.user import InvitedOrgUser from tests.conftest import ORGANISATION_ID, normalize_spaces diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 56c18dcfb..6f22eabdb 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -6,7 +6,7 @@ from flask import url_for from notifications_python_client.errors import HTTPError import app -from app.notify_client.models import InvitedUser +from app.models.user import InvitedUser from tests.conftest import ( SERVICE_ONE_ID, active_caseworking_user, diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index a97744d66..01819a4d4 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -9,7 +9,7 @@ from flask import url_for from freezegun import freeze_time from app.main.views.jobs import get_status_filters, get_time_left -from app.notify_client.models import Service +from app.models.service import Service from tests.conftest import ( SERVICE_ONE_ID, active_caseworking_user, diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 71cc6b836..e3a7b7590 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -5,7 +5,7 @@ from bs4 import BeautifulSoup from flask import url_for import app -from app.notify_client.models import InvitedUser +from app.models.user import InvitedUser from app.utils import is_gov_user from tests.conftest import ( SERVICE_ONE_ID, diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index d5cd1847c..5776afc98 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -6,7 +6,7 @@ from bs4 import BeautifulSoup from flask import session, url_for from flask_login import current_user -from app.notify_client.models import InvitedUser +from app.models.user import InvitedUser def test_render_register_returns_template_with_form(client): diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 85b4c69dd..52d4af555 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -563,13 +563,13 @@ def test_should_show_request_to_go_live_checklist( ) mock_templates = mocker.patch( - 'app.notify_client.models.Service.templates', + 'app.models.service.Service.templates', new_callable=PropertyMock, return_value=list(range(0, count_of_templates)), ) mock_templates_by_type = mocker.patch( - 'app.notify_client.models.Service.templates_by_type', + 'app.models.service.Service.templates_by_type', side_effect=_templates_by_type, ) @@ -683,12 +683,12 @@ def test_should_check_for_sms_sender_on_go_live( return_value=99, ) mock_templates = mocker.patch( - 'app.notify_client.models.Service.templates', + 'app.models.service.Service.templates', new_callable=PropertyMock, side_effect=partial(_templates_by_type, 'all'), ) mock_templates_by_type = mocker.patch( - 'app.notify_client.models.Service.templates_by_type', + 'app.models.service.Service.templates_by_type', side_effect=_templates_by_type, ) @@ -747,7 +747,7 @@ def test_should_check_for_mou_on_request_to_go_live( return_value=0, ) mocker.patch( - 'app.notify_client.models.Service.templates', + 'app.models.service.Service.templates', new_callable=PropertyMock, return_value=[], ) @@ -1022,16 +1022,16 @@ def test_ready_to_go_live( 'sms_sender_is_govuk', }: mocker.patch( - 'app.notify_client.models.Service.{}'.format(prop), + 'app.models.service.Service.{}'.format(prop), new_callable=PropertyMock ).return_value = locals()[prop] - assert app.notify_client.models.Service({ + assert app.models.service.Service({ 'id': SERVICE_ONE_ID }).go_live_checklist_completed_as_yes_no == expected_readyness assert list(app.main.views.service_settings._get_request_to_go_live_tags( - app.notify_client.models.Service({'id': SERVICE_ONE_ID}), + app.models.service.Service({'id': SERVICE_ONE_ID}), agreement_signed, )) == expected_tags diff --git a/tests/app/notify_client/test_notify_admin_api_client.py b/tests/app/notify_client/test_notify_admin_api_client.py index 3f127abad..2e542f87e 100644 --- a/tests/app/notify_client/test_notify_admin_api_client.py +++ b/tests/app/notify_client/test_notify_admin_api_client.py @@ -4,8 +4,8 @@ from unittest.mock import patch import pytest import werkzeug +from app.models.service import Service from app.notify_client import NotifyAdminAPIClient -from app.notify_client.models import Service from tests import service_json from tests.conftest import api_user_active, platform_admin_user, set_config diff --git a/tests/app/notify_client/test_user_client.py b/tests/app/notify_client/test_user_client.py index 33d0b1802..6eebb9cd2 100644 --- a/tests/app/notify_client/test_user_client.py +++ b/tests/app/notify_client/test_user_client.py @@ -3,7 +3,7 @@ from unittest.mock import call import pytest from app import invite_api_client, service_api_client, user_api_client -from app.notify_client.models import User +from app.models.user import User from tests import sample_uuid from tests.conftest import SERVICE_ONE_ID, api_user_pending @@ -51,7 +51,7 @@ def test_client_returns_count_of_users_with_manage_service( ) mocker.patch( - 'app.notify_client.models._get_service_id_from_view_args', + 'app.models.user._get_service_id_from_view_args', return_value=SERVICE_ONE_ID, ) @@ -221,7 +221,7 @@ def test_returns_value_from_cache( 'app.notify_client.RedisClient.set', ) mock_model = mocker.patch( - 'app.notify_client.models.User.__init__', + 'app.models.user.User.__init__', return_value=None, ) diff --git a/tests/conftest.py b/tests/conftest.py index 45edb97e6..e4a1e5b36 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -11,7 +11,7 @@ from notifications_python_client.errors import HTTPError from notifications_utils.url_safe_token import generate_token from app import create_app -from app.notify_client.models import InvitedOrgUser, InvitedUser, User +from app.models.user import InvitedOrgUser, InvitedUser, User from . import ( TestClient,