From efabf0e87d3babf3c0d3a55de8a189ce56b3bb59 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 27 Aug 2019 15:52:42 +0100 Subject: [PATCH 1/3] Refactor to avoid redefinition of org types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’re defining the list of org types in a few different places. This makes it more likely we’ll forget to update one of these places, thereby introducing a bug. This commit moves the definition to be on the organisation model, which feels like a sensible enough place for it. --- app/main/forms.py | 54 +++++++++++++++++--------------------- app/models/organisation.py | 11 ++++++++ 2 files changed, 35 insertions(+), 30 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index b33f3ef42..e5a084cd7 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -45,6 +45,7 @@ from app.main.validators import ( ValidEmail, ValidGovEmail, ) +from app.models.organisation import Organisation from app.models.roles_and_permissions import permissions, roles from app.utils import guess_name_from_email_address @@ -242,33 +243,23 @@ class ForgivingIntegerField(StringField): return super().__call__(value=value, **kwargs) -def organisation_type(label='Who runs this service?'): - return RadioField( - label, - choices=[ - ('central', 'Central government'), - ('local', 'Local government'), - ('nhs_central', 'NHS – central government agency or public body'), - ('nhs_local', 'NHS Trust or Clinical Commissioning Group'), - ('nhs_local', 'GP practice'), - ('emergency_service', 'Emergency service'), - ('school_or_college', 'School or college'), - ('other', 'Other'), - ], - validators=[DataRequired()], - ) - - -def nhs_organisation_type(label='Who runs this service?'): - return RadioField( - label, - choices=[ - ('nhs_central', 'NHS – central government agency or public body'), - ('nhs_local', 'NHS Trust or Clinical Commissioning Group'), - ('nhs_local', 'GP practice'), - ], - validators=[DataRequired()], - ) +class OrganisationTypeField(RadioField): + def __init__( + self, + *args, + include_only=None, + validators=None, + **kwargs + ): + super().__init__( + *args, + choices=[ + (value, label) for value, label in Organisation.TYPES + if not include_only or value in include_only + ], + validators=[DataRequired()] + (validators or []), + **kwargs + ) class FieldWithNoneOption(): @@ -544,7 +535,7 @@ class RenameOrganisationForm(StripWhitespaceForm): class OrganisationOrganisationTypeForm(StripWhitespaceForm): - organisation_type = organisation_type(label='What type of organisation is this?') + organisation_type = OrganisationTypeField('What type of organisation is this?') class OrganisationCrownStatusForm(StripWhitespaceForm): @@ -605,11 +596,14 @@ class CreateServiceForm(StripWhitespaceForm): validators=[ DataRequired(message='Can’t be empty') ]) - organisation_type = organisation_type() + organisation_type = OrganisationTypeField('Who runs this service?') class CreateNhsServiceForm(CreateServiceForm): - organisation_type = nhs_organisation_type() + organisation_type = OrganisationTypeField( + 'Who runs this service?', + include_only={'nhs_central', 'nhs_local'} + ) class NewOrganisationForm( diff --git a/app/models/organisation.py b/app/models/organisation.py index 5c6748f16..960b38b5f 100644 --- a/app/models/organisation.py +++ b/app/models/organisation.py @@ -7,6 +7,17 @@ from app.notify_client.organisations_api_client import organisations_client class Organisation(JSONModel): + 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_local', 'GP practice'), + ('emergency_service', 'Emergency service'), + ('school_or_college', 'School or college'), + ('other', 'Other'), + ) + ALLOWED_PROPERTIES = { 'id', 'name', From 38c2b32fa84c6056b20a193b9b3ca646928a465e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 27 Aug 2019 16:26:50 +0100 Subject: [PATCH 2/3] =?UTF-8?q?Add=20=E2=80=98GP=E2=80=99=20as=20an=20orga?= =?UTF-8?q?nisation=20type?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Although their allowances are the same as what we call `nhs_local` it makes more sense to store them separately because: - we already present them as two separate choices to the user - we may want to handle them differently in the future, eg in terms of what branding choices are available to them Once the API is updated we can start passing in this new value from the admin app. --- app/main/forms.py | 2 +- app/models/organisation.py | 2 +- tests/app/main/views/organisations/test_organisation.py | 4 ++-- tests/app/main/views/test_add_service.py | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index e5a084cd7..4dcf5818e 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -602,7 +602,7 @@ class CreateServiceForm(StripWhitespaceForm): class CreateNhsServiceForm(CreateServiceForm): organisation_type = OrganisationTypeField( 'Who runs this service?', - include_only={'nhs_central', 'nhs_local'} + include_only={'nhs_central', 'nhs_local', 'nhs_gp'}, ) diff --git a/app/models/organisation.py b/app/models/organisation.py index 960b38b5f..7ca83be26 100644 --- a/app/models/organisation.py +++ b/app/models/organisation.py @@ -12,7 +12,7 @@ class Organisation(JSONModel): ('local', 'Local government'), ('nhs_central', 'NHS – central government agency or public body'), ('nhs_local', 'NHS Trust or Clinical Commissioning Group'), - ('nhs_local', 'GP practice'), + ('nhs_gp', 'GP practice'), ('emergency_service', 'Emergency service'), ('school_or_college', 'School or college'), ('other', 'Other'), diff --git a/tests/app/main/views/organisations/test_organisation.py b/tests/app/main/views/organisations/test_organisation.py index cd7cd37fb..9a9817783 100644 --- a/tests/app/main/views/organisations/test_organisation.py +++ b/tests/app/main/views/organisations/test_organisation.py @@ -92,7 +92,7 @@ def test_page_to_create_new_organisation( ('radio', 'organisation_type', 'local'), ('radio', 'organisation_type', 'nhs_central'), ('radio', 'organisation_type', 'nhs_local'), - ('radio', 'organisation_type', 'nhs_local'), + ('radio', 'organisation_type', 'nhs_gp'), ('radio', 'organisation_type', 'emergency_service'), ('radio', 'organisation_type', 'school_or_college'), ('radio', 'organisation_type', 'other'), @@ -282,7 +282,7 @@ def test_organisation_settings_for_platform_admin( ('local', 'Local government'), ('nhs_central', 'NHS – central government agency or public body'), ('nhs_local', 'NHS Trust or Clinical Commissioning Group'), - ('nhs_local', 'GP practice'), + ('nhs_gp', 'GP practice'), ('emergency_service', 'Emergency service'), ('school_or_college', 'School or college'), ('other', 'Other'), diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 2a6910067..539a80132 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -55,7 +55,7 @@ def test_get_should_render_add_service_template( 'local', 'nhs_central', 'nhs_local', - 'nhs_local', + 'nhs_gp', 'emergency_service', 'school_or_college', 'other', @@ -205,7 +205,7 @@ def test_get_should_only_show_nhs_org_types_radios_if_user_has_nhs_email( ] == [ 'nhs_central', 'nhs_local', - 'nhs_local', + 'nhs_gp', ] From c9a32c73272f6a58da5c638fcf874c911ef4290c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 27 Aug 2019 16:32:30 +0100 Subject: [PATCH 3/3] Remove duplication of org type lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There’s a couple of places where we’re looking up the label for the type of organisation. Having this repeated in multiple places means it’s more likely we forget to update one of these places when making a change. This commit looks up from the tuple in the organisation model, which is where other code references this stuff from. This is only possible now that we don’t have duplicate keys (ie GP practice doesn’t share a key any more). --- app/models/organisation.py | 4 ++++ app/models/service.py | 4 ++++ .../organisations/organisation/settings/index.html | 10 +--------- app/templates/views/service-settings.html | 12 ++---------- 4 files changed, 11 insertions(+), 19 deletions(-) diff --git a/app/models/organisation.py b/app/models/organisation.py index 7ca83be26..e617e295a 100644 --- a/app/models/organisation.py +++ b/app/models/organisation.py @@ -109,6 +109,10 @@ class Organisation(JSONModel): def as_info_for_branding_request(self, fallback_domain): return self.name or 'Can’t tell (domain is {})'.format(fallback_domain) + @property + def organisation_type_label(self): + return dict(self.TYPES).get(self.organisation_type) + @property def crown_status_or_404(self): if self.crown is None: diff --git a/app/models/service.py b/app/models/service.py index f9dc2706b..5d4ce1df8 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -439,6 +439,10 @@ class Service(JSONModel): def organisation_type(self): return self.organisation.organisation_type or self._dict['organisation_type'] + @property + def organisation_type_label(self): + return dict(Organisation.TYPES).get(self.organisation_type) + @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/templates/views/organisations/organisation/settings/index.html b/app/templates/views/organisations/organisation/settings/index.html index f45a6997e..793274947 100644 --- a/app/templates/views/organisations/organisation/settings/index.html +++ b/app/templates/views/organisations/organisation/settings/index.html @@ -25,15 +25,7 @@ {% endcall %} {% call row() %} {{ text_field('Sector') }} - {{ optional_text_field({ - 'central': 'Central government', - 'local': 'Local government', - 'nhs_central': 'NHS – central government agency or public body', - 'nhs_local': 'NHS Trust, GP practice or Clinical Commissioning Group', - 'emergency_service': 'Emergency service', - 'school_or_college': 'School or college', - 'other': 'Other', - }.get(current_org.organisation_type)) }} + {{ optional_text_field(current_org.organisation_type_label) }} {{ edit_field( 'Change', url_for('.edit_organisation_type', org_id=current_org.id) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 60d6b7acc..6f4febc85 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -260,7 +260,7 @@

{% if current_user.has_permissions('manage_service') %} - To remove these restrictions, you can send us a + To remove these restrictions, you can send us a request to go live. {% else %} Your service manager can ask to have these restrictions removed. @@ -324,15 +324,7 @@ {% endif %} {% if current_service.organisation_type %}

- {{ { - 'central': 'Central government', - 'local': 'Local government', - 'nhs_central': 'NHS – central government agency or public body', - 'nhs_local': 'NHS Trust, GP practice or Clinical Commissioning Group', - 'emergency_service': 'Emergency service', - 'school_or_college': 'School or college', - 'other': 'Other' - }.get(current_service.organisation_type) }} + {{ current_service.organisation_type_label }}
{% endif %} {% endcall %}