From 20f857753a7e1ee27c9330aafacac6001acc6c2a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 12 Sep 2019 15:03:32 +0100 Subject: [PATCH] Use constants for organisation type This reduces the chances of making a typo, because doing so will raise an exception rather than fail silently. --- app/main/forms.py | 18 ++++++++++++------ app/main/views/agreement.py | 5 +++-- app/main/views/organisations.py | 6 +++--- app/models/organisation.py | 25 +++++++++++++++++-------- app/models/service.py | 9 ++++++--- 5 files changed, 41 insertions(+), 22 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 270da37a0..9762917b5 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1375,14 +1375,14 @@ class BrandingOptionsEmail(StripWhitespaceForm): def get_available_choices(service): if ( - service.organisation_type == 'central' and + service.organisation_type == Organisation.TYPE_CENTRAL and service.organisation.email_branding_id is None and service.email_branding_id is not None ): yield ('govuk', 'GOV.UK') if ( - service.organisation_type == 'central' and + service.organisation_type == Organisation.TYPE_CENTRAL and service.organisation and service.organisation.email_branding_id is None and service.email_branding_name.lower() != 'GOV.UK and {}'.format(service.organisation.name).lower() @@ -1390,15 +1390,21 @@ class BrandingOptionsEmail(StripWhitespaceForm): yield ('govuk_and_org', 'GOV.UK and {}'.format(service.organisation.name)) if ( - service.organisation_type in {'nhs_local', 'nhs_central', 'nhs_gp'} and - service.email_branding_name != 'NHS' + service.organisation_type in { + Organisation.TYPE_NHS_CENTRAL, + Organisation.TYPE_NHS_LOCAL, + Organisation.TYPE_NHS_GP, + } and service.email_branding_name != 'NHS' ): yield ('nhs', 'NHS') if ( service.organisation and - service.organisation_type not in {'nhs_local', 'nhs_central', 'nhs_gp'} and - ( + service.organisation_type not in { + Organisation.TYPE_NHS_LOCAL, + Organisation.TYPE_NHS_CENTRAL, + Organisation.TYPE_NHS_GP, + } and ( service.email_branding_id is None or service.email_branding_id != service.organisation.email_branding_id ) diff --git a/app/main/views/agreement.py b/app/main/views/agreement.py index b03aecd87..6fccac17b 100644 --- a/app/main/views/agreement.py +++ b/app/main/views/agreement.py @@ -6,6 +6,7 @@ from flask_login import current_user from app import current_service from app.main import main from app.main.forms import AcceptAgreementForm +from app.models.organisation import Organisation from app.s3_client.s3_mou_client import get_mou from app.utils import user_has_permissions @@ -14,11 +15,11 @@ from app.utils import user_has_permissions @user_has_permissions('manage_service') def service_agreement(service_id): if not current_service.organisation: - if current_service.organisation_type == 'nhs_gp': + if current_service.organisation_type == Organisation.TYPE_NHS_GP: return redirect( url_for('main.add_organisation_from_gp_service', service_id=current_service.id) ) - if current_service.organisation_type == 'nhs_local': + if current_service.organisation_type == Organisation.TYPE_NHS_LOCAL: return redirect( url_for('main.add_organisation_from_nhs_local_service', service_id=current_service.id) ) diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index f2ed69ee1..3a7b66f10 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -69,7 +69,7 @@ def add_organisation(): @main.route('/services//add-gp-organisation', methods=['GET', 'POST']) @user_has_permissions('manage_service') def add_organisation_from_gp_service(service_id): - if (not current_service.organisation_type == 'nhs_gp') or current_service.organisation: + if (not current_service.organisation_type == Organisation.TYPE_NHS_GP) or current_service.organisation: abort(403) form = AddGPOrganisationForm(service_name=current_service.name) @@ -97,13 +97,13 @@ def add_organisation_from_gp_service(service_id): @main.route('/services//add-nhs-local-organisation', methods=['GET', 'POST']) @user_has_permissions('manage_service') def add_organisation_from_nhs_local_service(service_id): - if (not current_service.organisation_type == 'nhs_local') or current_service.organisation: + if (not current_service.organisation_type == Organisation.TYPE_NHS_LOCAL) or current_service.organisation: abort(403) form = AddNHSLocalOrganisationForm(organisation_choices=[ (organisation.id, organisation.name) for organisation in Organisations() - if organisation.organisation_type == 'nhs_local' + if organisation.organisation_type == Organisation.TYPE_NHS_LOCAL ]) search_form = SearchByNameForm() diff --git a/app/models/organisation.py b/app/models/organisation.py index 1d5fcb2e3..50ccfe059 100644 --- a/app/models/organisation.py +++ b/app/models/organisation.py @@ -9,15 +9,24 @@ from app.notify_client.organisations_api_client import organisations_client class Organisation(JSONModel): + TYPE_CENTRAL = 'central' + TYPE_LOCAL = 'local' + TYPE_NHS_CENTRAL = 'nhs_central' + TYPE_NHS_LOCAL = 'nhs_local' + TYPE_NHS_GP = 'nhs_gp' + TYPE_EMERGENCY_SERVICE = 'emergency_service' + TYPE_SCHOOL_OR_COLLEGE = 'school_or_college' + TYPE_OTHER = 'other' + TYPES = ( - ('central', 'Central government'), - ('local', 'Local government'), - ('nhs_central', 'NHS – central government agency or public body'), - ('nhs_local', 'NHS Trust or Clinical Commissioning Group'), - ('nhs_gp', 'GP practice'), - ('emergency_service', 'Emergency service'), - ('school_or_college', 'School or college'), - ('other', 'Other'), + (TYPE_CENTRAL, 'Central government'), + (TYPE_LOCAL, 'Local government'), + (TYPE_NHS_CENTRAL, 'NHS – central government agency or public body'), + (TYPE_NHS_LOCAL, 'NHS Trust or Clinical Commissioning Group'), + (TYPE_NHS_GP, 'GP practice'), + (TYPE_EMERGENCY_SERVICE, 'Emergency service'), + (TYPE_SCHOOL_OR_COLLEGE, 'School or college'), + (TYPE_OTHER, 'Other'), ) ALLOWED_PROPERTIES = { diff --git a/app/models/service.py b/app/models/service.py index 1d5e26af6..3e3e93fd6 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -270,7 +270,7 @@ class Service(JSONModel): @property def shouldnt_use_govuk_as_sms_sender(self): - return self.organisation_type != 'central' + return self.organisation_type != Organisation.TYPE_CENTRAL @cached_property def sms_senders(self): @@ -419,7 +419,7 @@ class Service(JSONModel): @property def needs_to_change_email_branding(self): - return self.email_branding_id is None and self.organisation_type != 'central' + return self.email_branding_id is None and self.organisation_type != Organisation.TYPE_CENTRAL @property def letter_branding_id(self): @@ -599,7 +599,10 @@ class Service(JSONModel): def able_to_accept_agreement(self): return ( self.organisation.agreement_signed is not None - or self.organisation_type in {'nhs_gp', 'nhs_local'} + or self.organisation_type in { + Organisation.TYPE_NHS_GP, + Organisation.TYPE_NHS_LOCAL, + } ) @property