diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index e2f9b27cd..49e381248 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -428,6 +428,7 @@ def clear_cache(): 'organisations', 'domains', 'live-service-and-organisation-counts', + 'organisation-????????-????-????-????-????????????-name', ]), ]) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 78e371363..73766ac6b 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -991,15 +991,14 @@ def service_preview_letter_branding(service_id): def link_service_to_organisation(service_id): all_organisations = organisations_client.get_organisations() - current_linked_organisation = organisations_client.get_service_organisation(service_id).get('id', None) form = LinkOrganisationsForm( choices=convert_dictionary_to_wtforms_choices_format(all_organisations, 'id', 'name'), - organisations=current_linked_organisation + organisations=current_service.organisation_id ) if form.validate_on_submit(): - if form.organisations.data != current_linked_organisation: + if form.organisations.data != current_service.organisation_id: organisations_client.update_service_organisation( service_id, form.organisations.data diff --git a/app/models/organisation.py b/app/models/organisation.py index 8d1390250..4028a0d51 100644 --- a/app/models/organisation.py +++ b/app/models/organisation.py @@ -50,6 +50,8 @@ class Organisation(JSONModel): @classmethod def from_id(cls, org_id): + if not org_id: + return cls({}) return cls(organisations_client.get_organisation(org_id)) @classmethod diff --git a/app/models/service.py b/app/models/service.py index c053438cc..241d325eb 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -18,6 +18,7 @@ from app.notify_client.inbound_number_client import inbound_number_client from app.notify_client.invite_api_client import invite_api_client from app.notify_client.job_api_client import job_api_client from app.notify_client.letter_branding_client import letter_branding_client +from app.notify_client.organisations_api_client import organisations_client from app.notify_client.service_api_client import service_api_client from app.notify_client.template_folder_api_client import ( template_folder_api_client, @@ -459,7 +460,7 @@ class Service(JSONModel): @cached_property def organisation(self): - return Organisation.from_service(self.id) + return Organisation.from_id(self.organisation_id) @property def organisation_id(self): @@ -469,6 +470,12 @@ class Service(JSONModel): def organisation_type(self): return self.organisation.organisation_type or self._dict['organisation_type'] + @property + def organisation_name(self): + if not self.organisation: + return None + return organisations_client.get_organisation_name(self.organisation_id) + @property def organisation_type_label(self): return dict(Organisation.TYPES).get(self.organisation_type) diff --git a/app/notify_client/organisations_api_client.py b/app/notify_client/organisations_api_client.py index d1c309081..f30fe009b 100644 --- a/app/notify_client/organisations_api_client.py +++ b/app/notify_client/organisations_api_client.py @@ -22,6 +22,10 @@ class OrganisationsClient(NotifyAdminAPIClient): def get_organisation(self, org_id): return self.get(url='/organisations/{}'.format(org_id)) + @cache.set('organisation-{org_id}-name') + def get_organisation_name(self, org_id): + return self.get_organisation(org_id)['name'] + def get_organisation_by_domain(self, domain): try: return self.get( @@ -54,6 +58,7 @@ class OrganisationsClient(NotifyAdminAPIClient): return api_response + @cache.delete('organisation-{org_id}-name') def update_organisation_name(self, org_id, name): return self.update_organisation(org_id, name=name) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 7b7213e3a..767d611fe 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -316,7 +316,7 @@ {% call field() %} {% if current_service.organisation_id %} - {{ current_service.organisation.name }} + {{ current_service.organisation_name }} {% else %} Not set diff --git a/app/templates/withnav_template.html b/app/templates/withnav_template.html index 6c24a22fa..114bc8bdf 100644 --- a/app/templates/withnav_template.html +++ b/app/templates/withnav_template.html @@ -12,7 +12,7 @@ {% if current_service.organisation_id %} {% if current_user.platform_admin or (current_user.belongs_to_organisation(current_service.organisation_id) and current_service.live) %} - {{ current_service.organisation.name }} + {{ current_service.organisation_name }} {% endif %} {% endif %}
diff --git a/tests/__init__.py b/tests/__init__.py index 675204cf2..f1a8d6626 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -204,7 +204,7 @@ def organisation_json( agreement_signed_version=None, agreement_signed_on_behalf_of_name=None, agreement_signed_on_behalf_of_email_address=None, - organisation_type='', + organisation_type='central', request_to_go_live_notes=None, ): if users is None: diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index 7a16e5012..c2f8989b3 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -298,7 +298,7 @@ def test_services_pages_that_org_users_are_allowed_to_see( mock_get_invites_for_service, mock_get_users_by_service, mock_get_template_folders, - mock_get_service_organisation, + mock_get_organisation, mock_has_jobs, user_services, user_organisations, @@ -352,7 +352,7 @@ def test_service_navigation_for_org_user( mock_get_service, mock_get_invites_for_service, mock_get_users_by_service, - mock_get_service_organisation, + mock_get_organisation, ): api_user_active['services'] = [] api_user_active['organisations'] = [ORGANISATION_ID] @@ -412,7 +412,7 @@ def test_service_user_without_manage_service_permission_can_see_usage_page_when_ mock_get_service, mock_get_invites_for_service, mock_get_users_by_service, - mock_get_service_organisation, + mock_get_organisation, mock_get_service_templates, mock_get_template_folders, user_organisations, diff --git a/tests/app/main/views/organisations/test_organisation.py b/tests/app/main/views/organisations/test_organisation.py index 8085e30ef..8d1179a79 100644 --- a/tests/app/main/views/organisations/test_organisation.py +++ b/tests/app/main/views/organisations/test_organisation.py @@ -1,4 +1,4 @@ -from unittest.mock import ANY, Mock +from unittest.mock import ANY, Mock, PropertyMock import pytest from bs4 import BeautifulSoup @@ -200,7 +200,12 @@ def test_gps_can_create_own_organisations( organisation, expected_status, ): - mocker.patch('app.organisations_client.get_service_organisation', return_value=organisation) + mocker.patch( + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + mocker.patch('app.organisations_client.get_organisation', return_value=organisation) service_one['organisation_type'] = organisation_type page = client_request.get( @@ -234,7 +239,12 @@ def test_nhs_local_can_create_own_organisations( organisation, expected_status, ): - mocker.patch('app.organisations_client.get_service_organisation', return_value=organisation) + mocker.patch( + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + mocker.patch('app.organisations_client.get_organisation', return_value=organisation) mocker.patch( 'app.models.organisation.Organisations.client_method', return_value=[ @@ -300,7 +310,6 @@ def test_gps_can_name_their_organisation( data, expected_service_name, ): - mocker.patch('app.organisations_client.get_service_organisation', return_value=None) service_one['organisation_type'] = 'nhs_gp' mock_create_organisation = mocker.patch( 'app.organisations_client.create_organisation', @@ -350,7 +359,6 @@ def test_validation_of_gps_creating_organisations( data, expected_error, ): - mocker.patch('app.organisations_client.get_service_organisation', return_value=None) service_one['organisation_type'] = 'nhs_gp' page = client_request.post( '.add_organisation_from_gp_service', @@ -368,7 +376,6 @@ def test_nhs_local_assigns_to_selected_organisation( mock_get_organisation, mock_update_service_organisation, ): - mocker.patch('app.organisations_client.get_service_organisation', return_value=None) mocker.patch( 'app.models.organisation.Organisations.client_method', return_value=[ @@ -652,8 +659,8 @@ def test_organisation_settings_for_platform_admin( ): expected_rows = [ 'Label Value Action', - 'Name Org 1 Change', - 'Sector Not set Change', + 'Name Test organisation Change', + 'Sector Central government Change', 'Crown organisation Yes Change', 'Data sharing and financial agreement Not signed Change', 'Request to go live notes None Change', @@ -686,7 +693,7 @@ def test_organisation_settings_for_platform_admin( ('school_or_college', 'School or college'), ('other', 'Other'), ), - None, + 'central', ), ( '.edit_organisation_crown_status', diff --git a/tests/app/main/views/organisations/test_organisation_invites.py b/tests/app/main/views/organisations/test_organisation_invites.py index aa88b27e6..3adbfb62e 100644 --- a/tests/app/main/views/organisations/test_organisation_invites.py +++ b/tests/app/main/views/organisations/test_organisation_invites.py @@ -118,7 +118,7 @@ def test_cancelled_invite_opened_by_user( ) == 'Test User decided to cancel this invitation.' assert normalize_spaces( page.select('main p')[1].text - ) == 'If you need access to Org 1, you’ll have to ask them to invite you again.' + ) == 'If you need access to Test organisation, you’ll have to ask them to invite you again.' mock_get_user.assert_called_once_with(fake_uuid) mock_get_organisation.assert_called_once_with(ORGANISATION_ID) diff --git a/tests/app/main/views/service_settings/test_service_setting_permissions.py b/tests/app/main/views/service_settings/test_service_setting_permissions.py index a0d969bf0..be11ad602 100644 --- a/tests/app/main/views/service_settings/test_service_setting_permissions.py +++ b/tests/app/main/views/service_settings/test_service_setting_permissions.py @@ -1,10 +1,11 @@ import functools +from unittest.mock import PropertyMock import pytest from flask import url_for from app.main.views.service_settings import PLATFORM_ADMIN_SERVICE_PERMISSIONS -from tests.conftest import normalize_spaces +from tests.conftest import ORGANISATION_ID, normalize_spaces @pytest.fixture @@ -14,7 +15,7 @@ def get_service_settings_page( service_one, mock_get_inbound_number_for_service, mock_get_all_letter_branding, - mock_get_service_organisation, + mock_get_organisation, mock_get_free_sms_fragment_limit, no_reply_to_email_addresses, no_letter_contact_blocks, @@ -76,9 +77,22 @@ def test_service_set_permission( ({'permissions': ['letter']}, '.service_set_permission', {'permission': 'international_letters'}, 'Send international letters Off Change'), ]) -def test_service_setting_toggles_show(get_service_settings_page, service_one, service_fields, endpoint, kwargs, text): +def test_service_setting_toggles_show( + mocker, + get_service_settings_page, + service_one, + service_fields, + endpoint, + kwargs, + text, +): link_url = url_for(endpoint, **kwargs, service_id=service_one['id']) service_one.update(service_fields) + mocker.patch( + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) page = get_service_settings_page() assert normalize_spaces(page.find('a', {'href': link_url}).find_parent('tr').text.strip()) == text @@ -145,7 +159,7 @@ def test_normal_user_doesnt_see_any_platform_admin_settings( service_one, no_reply_to_email_addresses, no_letter_contact_blocks, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mock_get_all_letter_branding, mock_get_inbound_number_for_service, diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 36be60280..63192c38d 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -233,11 +233,12 @@ def test_get_should_only_show_nhs_org_types_radios_if_user_has_nhs_email( ]) def test_should_add_service_and_redirect_to_dashboard_when_existing_service( app_, + mocker, client_request, mock_create_service, mock_create_service_template, mock_get_services, - mock_get_organisation_by_domain, + mock_get_no_organisation_by_domain, api_user_active, organisation_type, free_allowance, diff --git a/tests/app/main/views/test_agreement.py b/tests/app/main/views/test_agreement.py index 24ae7ebc3..3e0113a8d 100644 --- a/tests/app/main/views/test_agreement.py +++ b/tests/app/main/views/test_agreement.py @@ -1,13 +1,13 @@ from functools import partial from io import BytesIO -from unittest.mock import call +from unittest.mock import PropertyMock, call import pytest from flask import url_for from freezegun import freeze_time from tests import organisation_json -from tests.conftest import SERVICE_ONE_ID, normalize_spaces +from tests.conftest import ORGANISATION_ID, SERVICE_ONE_ID, normalize_spaces class _MockS3Object(): @@ -90,11 +90,16 @@ def test_show_agreement_page( crown, expected_links, ): + mocker.patch( + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) org = organisation_json( crown=crown, agreement_signed=agreement_signed ) - mocker.patch('app.organisations_client.get_service_organisation', return_value=org) + mocker.patch('app.organisations_client.get_organisation', return_value=org) page = client_request.get('main.service_agreement', service_id=SERVICE_ONE_ID) links = page.select('main .govuk-grid-column-five-sixths a') @@ -118,7 +123,6 @@ def test_unknown_gps_and_trusts_are_redirected( org_type, expected_endpoint, ): - mocker.patch('app.organisations_client.get_service_organisation', return_value=None) service_one['organisation_id'] = None service_one['organisation_type'] = org_type client_request.get( @@ -156,7 +160,12 @@ def test_download_service_agreement( expected_file_served, ): mocker.patch( - 'app.models.organisation.organisations_client.get_service_organisation', + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + mocker.patch( + 'app.models.organisation.organisations_client.get_organisation', return_value=organisation_json( crown=crown ) @@ -187,8 +196,13 @@ def test_download_service_agreement( def test_show_accept_agreement_page( client_request, mocker, - mock_get_service_organisation, + mock_get_organisation, ): + mocker.patch( + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) page = client_request.get('main.service_accept_agreement', service_id=SERVICE_ONE_ID) assert [ @@ -244,10 +258,15 @@ def test_show_accept_agreement_page( def test_accept_agreement_page_populates( client_request, mocker, - mock_get_service_organisation, + mock_get_organisation, ): mocker.patch( - 'app.models.organisation.organisations_client.get_service_organisation', + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + mocker.patch( + 'app.models.organisation.organisations_client.get_organisation', return_value=organisation_json( agreement_signed_version='1.2', agreement_signed_on_behalf_of_name='Firstname Lastname', @@ -328,11 +347,17 @@ def test_accept_agreement_page_populates( )) def test_accept_agreement_page_validates( + mocker, client_request, - mock_get_service_organisation, + mock_get_organisation, data, expected_errors, ): + mocker.patch( + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) page = client_request.post( 'main.service_accept_agreement', service_id=SERVICE_ONE_ID, @@ -353,7 +378,7 @@ def test_accept_agreement_page_validates( 'on_behalf_of_email': 'test@example.com', }, call( - '7aa5d4e9-4385-4488-a489-07812ba13383', + ORGANISATION_ID, agreement_signed_version=1.2, agreement_signed_on_behalf_of_name='Firstname Lastname', agreement_signed_on_behalf_of_email_address='test@example.com', @@ -368,7 +393,7 @@ def test_accept_agreement_page_validates( 'on_behalf_of_email': 'test@example.com', }, call( - '7aa5d4e9-4385-4488-a489-07812ba13383', + ORGANISATION_ID, agreement_signed_version=1.2, agreement_signed_on_behalf_of_name='', agreement_signed_on_behalf_of_email_address='', @@ -383,7 +408,7 @@ def test_accept_agreement_page_validates( 'on_behalf_of_email': '', }, call( - '7aa5d4e9-4385-4488-a489-07812ba13383', + ORGANISATION_ID, agreement_signed_version=1.2, agreement_signed_on_behalf_of_name='', agreement_signed_on_behalf_of_email_address='', @@ -392,12 +417,18 @@ def test_accept_agreement_page_validates( ), )) def test_accept_agreement_page_persists( + mocker, client_request, - mock_get_service_organisation, + mock_get_organisation, mock_update_organisation, data, expected_persisted, ): + mocker.patch( + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) client_request.post( 'main.service_accept_agreement', service_id=SERVICE_ONE_ID, @@ -433,7 +464,12 @@ def test_show_confirm_agreement_page( expected_paragraph, ): mocker.patch( - 'app.models.organisation.organisations_client.get_service_organisation', + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + mocker.patch( + 'app.models.organisation.organisations_client.get_organisation', return_value=organisation_json( agreement_signed_version='1.2', agreement_signed_on_behalf_of_name=name, @@ -447,7 +483,7 @@ def test_show_confirm_agreement_page( @pytest.mark.parametrize('http_method', ('get', 'post')) def test_confirm_agreement_page_403s_if_previous_step_not_taken( client_request, - mock_get_service_organisation, + mock_get_organisation, http_method, ): getattr(client_request, http_method)( @@ -465,7 +501,12 @@ def test_confirm_agreement_page_persists( fake_uuid, ): mocker.patch( - 'app.models.organisation.organisations_client.get_service_organisation', + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + mocker.patch( + 'app.models.organisation.organisations_client.get_organisation', return_value=organisation_json(agreement_signed_version='1.2') ) client_request.post( diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index b83c16b28..8663e29ca 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -17,6 +17,7 @@ from app.main.views.dashboard import ( get_tuples_of_financial_years, ) from tests import ( + organisation_json, service_json, validate_route_permission, validate_route_permission_with_client, @@ -1554,7 +1555,9 @@ def test_org_breadcrumbs_show_if_user_is_a_member_of_the_services_org( organisation_id=ORGANISATION_ID) mocker.patch('app.service_api_client.get_service', return_value={'data': service_one_json}) - mocker.patch('app.models.service.Organisation') + mocker.patch('app.organisations_client.get_organisation', return_value=organisation_json( + id_=ORGANISATION_ID, + )) page = client_request.get('main.service_dashboard', service_id=SERVICE_ONE_ID) assert page.select_one('.navigation-organisation-link')['href'] == url_for( @@ -1604,7 +1607,9 @@ def test_org_breadcrumbs_show_if_user_is_platform_admin( organisation_id=ORGANISATION_ID) mocker.patch('app.service_api_client.get_service', return_value={'data': service_one_json}) - mocker.patch('app.models.service.Organisation') + mocker.patch('app.organisations_client.get_organisation', return_value=organisation_json( + id_=ORGANISATION_ID, + )) response = platform_admin_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index 28effd4dc..ea6168963 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -763,6 +763,7 @@ def test_clear_cache_shows_form(client_request, platform_admin_user, mocker): call('organisations'), call('domains'), call('live-service-and-organisation-counts'), + call('organisation-????????-????-????-????-????????????-name'), ], 'Removed 3 organisation objects from redis'), )) def test_clear_cache_submits_and_tells_you_how_many_things_were_deleted( diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 9990ddf67..a906be98d 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -99,7 +99,7 @@ def mock_get_service_settings_page_common( 'Label Value Action', 'Live Off Change', 'Count in list of live services Yes Change', - 'Organisation Test Organisation Central government Change', + 'Organisation Test organisation Central government Change', 'Free text message allowance 250,000 Change', 'Email branding GOV.UK Change', 'Letter branding Not set Change', @@ -114,7 +114,7 @@ def test_should_show_overview( api_user_active, no_reply_to_email_addresses, no_letter_contact_blocks, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, user, expected_rows, @@ -152,7 +152,7 @@ def test_no_go_live_link_for_service_without_organisation( platform_admin_user, mock_get_service_settings_page_common, ): - mocker.patch('app.organisations_client.get_service_organisation', return_value=None) + mocker.patch('app.organisations_client.get_organisation', return_value=None) client_request.login(platform_admin_user) page = client_request.get('main.service_settings', service_id=SERVICE_ONE_ID) @@ -174,7 +174,7 @@ def test_organisation_name_links_to_org_dashboard( single_sms_sender, mock_get_service_settings_page_common, mocker, - mock_get_service_organisation, + mock_get_organisation, ): service_one = service_json(SERVICE_ONE_ID, permissions=['sms', 'email'], @@ -188,7 +188,7 @@ def test_organisation_name_links_to_org_dashboard( org_row = find_element_by_tag_and_partial_text(response, tag='tr', string='Organisation') assert org_row.find('a')['href'] == url_for('main.organisation_dashboard', org_id=ORGANISATION_ID) - assert normalize_spaces(org_row.find('a').text) == 'Test Organisation' + assert normalize_spaces(org_row.find('a').text) == 'Test organisation' @pytest.mark.parametrize('service_contact_link,expected_text', [ @@ -203,7 +203,7 @@ def test_send_files_by_email_row_on_settings_page( single_sms_sender, mock_get_service_settings_page_common, mocker, - mock_get_service_organisation, + mock_get_organisation, service_contact_link, expected_text ): @@ -296,7 +296,7 @@ def test_should_show_overview_for_service_with_more_things_set( single_reply_to_email_address, single_letter_contact_block, single_sms_sender, - mock_get_service_organisation, + mock_get_organisation, mock_get_email_branding, mock_get_service_settings_page_common, permissions, @@ -318,7 +318,7 @@ def test_if_cant_send_letters_then_cant_see_letter_contact_block( service_one, single_reply_to_email_address, no_letter_contact_blocks, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mock_get_service_settings_page_common, ): @@ -331,7 +331,7 @@ def test_letter_contact_block_shows_none_if_not_set( service_one, single_reply_to_email_address, no_letter_contact_blocks, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mock_get_service_settings_page_common, ): @@ -352,7 +352,7 @@ def test_escapes_letter_contact_block( mocker, single_reply_to_email_address, single_sms_sender, - mock_get_service_organisation, + mock_get_organisation, injected_letter_contact_block, mock_get_service_settings_page_common, ): @@ -463,7 +463,7 @@ def test_show_restricted_service( client_request, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mock_get_service_settings_page_common, user, @@ -522,7 +522,7 @@ def test_show_live_service( mock_get_live_service, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mock_get_service_settings_page_common, ): @@ -716,7 +716,7 @@ def test_should_check_if_estimated_volumes_provided( single_reply_to_email_address, mock_get_service_templates, mock_get_users_by_service, - mock_get_service_organisation, + mock_get_organisation, mock_get_invites_for_service, volumes, consent_to_research, @@ -784,7 +784,6 @@ def test_should_check_for_sending_things_right( mocker, service_one, fake_uuid, - mock_get_service_organisation, single_sms_sender, count_of_users_with_manage_service, count_of_invites_with_manage_service, @@ -842,6 +841,11 @@ def test_should_check_for_sending_things_right( 'app.models.service.Service.get_templates', side_effect=_templates_by_type, ) + mocker.patch( + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=None, + ) mock_get_reply_to_email_addresses = mocker.patch( 'app.main.views.service_settings.service_api_client.get_reply_to_email_addresses', @@ -862,7 +866,6 @@ def test_should_check_for_sending_things_right( assert page.h1.text == 'Before you request to go live' checklist_items = page.select('.task-list .task-list-item') - assert normalize_spaces(checklist_items[1].text) == expected_user_checklist_item assert normalize_spaces(checklist_items[2].text) == expected_templates_checklist_item assert normalize_spaces(checklist_items[3].text) == expected_reply_to_checklist_item @@ -887,7 +890,7 @@ def test_should_not_show_go_live_button_if_checklist_not_complete( mocker, mock_get_service_templates, mock_get_users_by_service, - mock_get_service_organisation, + mock_get_organisation, mock_get_invites_for_service, single_sms_sender, checklist_completed, @@ -899,6 +902,11 @@ def test_should_not_show_go_live_button_if_checklist_not_complete( new_callable=PropertyMock, return_value=checklist_completed, ) + mocker.patch( + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) mocker.patch( 'app.models.organisation.Organisation.agreement_signed', new_callable=PropertyMock, @@ -1028,7 +1036,7 @@ def test_should_check_for_sms_sender_on_go_live( client_request, service_one, mocker, - mock_get_service_organisation, + mock_get_organisation, mock_get_invites_for_service, organisation_type, count_of_sms_templates, @@ -1137,7 +1145,12 @@ def test_should_check_for_mou_on_request_to_go_live( ) mocker.patch( - 'app.organisations_client.get_service_organisation', + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + mocker.patch( + 'app.organisations_client.get_organisation', return_value=organisation_json(agreement_signed=agreement_signed) ) page = client_request.get( @@ -1187,9 +1200,16 @@ def test_gp_without_organisation_is_shown_agreement_step( return_value=None, ) - mocker.patch('app.organisations_client.get_service_organisation', return_value=None) - service_one['organisation_id'] = None - service_one['organisation_type'] = organisation_type + mocker.patch( + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=None, + ) + mocker.patch( + 'app.models.service.Service.organisation_type', + new_callable=PropertyMock, + return_value=organisation_type, + ) page = client_request.get( 'main.request_to_go_live', service_id=SERVICE_ONE_ID @@ -1208,7 +1228,7 @@ def test_non_gov_user_is_told_they_cant_go_live( mock_get_invites_for_service, mocker, mock_get_organisations, - mock_get_service_organisation, + mock_get_organisation, ): mocker.patch( 'app.models.service.Service.has_team_members', @@ -1492,11 +1512,6 @@ def test_should_redirect_after_request_to_go_live( formatted_displayed_volumes, extra_tags, ): - mocker.patch( - 'app.organisations_client.get_service_organisation', - return_value=organisation_json(name=None, agreement_signed=None) - ) - for channel, volume in volumes: mocker.patch( 'app.models.service.Service.volume_{}'.format(channel), @@ -1573,7 +1588,12 @@ def test_request_to_go_live_displays_go_live_notes_in_zendesk_ticket( go_live_note = 'This service is not allowed to go live' mocker.patch( - 'app.organisations_client.get_service_organisation', + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + mocker.patch( + 'app.organisations_client.get_organisation', side_effect=lambda org_id: organisation_json( ORGANISATION_ID, 'Org 1', @@ -1621,10 +1641,6 @@ def test_should_be_able_to_request_to_go_live_with_no_organisation( mock_update_service, mock_get_invites_without_manage_permission, ): - get_service_organisation = mocker.patch( - 'app.organisations_client.get_service_organisation', - return_value=None, - ) for channel in {'email', 'sms', 'letter'}: mocker.patch( 'app.models.service.Service.volume_{}'.format(channel), @@ -1641,7 +1657,6 @@ def test_should_be_able_to_request_to_go_live_with_no_organisation( ) assert mock_post.called is True - get_service_organisation.assert_called_once_with(SERVICE_ONE_ID) @pytest.mark.parametrize( @@ -1808,7 +1823,12 @@ def test_ready_to_go_live( expected_tags, ): mocker.patch( - 'app.organisations_client.get_service_organisation', + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + mocker.patch( + 'app.organisations_client.get_organisation', return_value=organisation_json(agreement_signed=agreement_signed) ) @@ -1863,7 +1883,7 @@ def test_route_permissions( service_one, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, mock_get_invites_for_service, single_sms_sender, route, @@ -1926,7 +1946,7 @@ def test_route_for_platform_admin( service_one, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, route, mock_get_service_settings_page_common, @@ -1948,7 +1968,7 @@ def test_and_more_hint_appears_on_settings_with_more_than_just_a_single_sender( service_one, multiple_reply_to_email_addresses, multiple_letter_contact_blocks, - mock_get_service_organisation, + mock_get_organisation, multiple_sms_senders, mock_get_service_settings_page_common, ): @@ -1978,7 +1998,7 @@ def test_api_ids_dont_show_on_option_pages_with_a_single_sender( client_request, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, sender_list_page, index, @@ -2962,7 +2982,7 @@ def test_shows_research_mode_indicator( mocker, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mock_get_service_settings_page_common, ): @@ -2982,7 +3002,7 @@ def test_does_not_show_research_mode_indicator( client_request, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mock_get_service_settings_page_common, ): @@ -3789,7 +3809,7 @@ def test_archive_service_prompts_user( mocker, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mock_get_service_settings_page_common, user, @@ -3825,7 +3845,7 @@ def test_cant_archive_inactive_service( service_one, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mock_get_service_settings_page_common ): @@ -3859,7 +3879,7 @@ def test_suspend_service_prompts_user( mocker, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mock_get_service_settings_page_common, ): @@ -3879,7 +3899,7 @@ def test_cant_suspend_inactive_service( service_one, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mock_get_service_settings_page_common, ): @@ -3897,7 +3917,7 @@ def test_resume_service_after_confirm( service_one, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, mocker, mock_get_inbound_number_for_service, ): @@ -3916,7 +3936,7 @@ def test_resume_service_prompts_user( service_one, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mocker, mock_get_service_settings_page_common, @@ -3938,7 +3958,7 @@ def test_cant_resume_active_service( service_one, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mock_get_service_settings_page_common ): @@ -3979,7 +3999,7 @@ def test_send_files_by_email_contact_details_updates_contact_details_and_redirec service_one, mock_update_service, mock_get_service_settings_page_common, - mock_get_service_organisation, + mock_get_organisation, no_reply_to_email_addresses, no_letter_contact_blocks, single_sms_sender, @@ -4007,7 +4027,7 @@ def test_send_files_by_email_contact_details_uses_the_selected_field_when_multip service_one, mock_update_service, mock_get_service_settings_page_common, - mock_get_service_organisation, + mock_get_organisation, no_reply_to_email_addresses, no_letter_contact_blocks, single_sms_sender, @@ -4101,7 +4121,7 @@ def test_contact_link_is_not_displayed_without_the_upload_document_permission( client_request, service_one, mock_get_service_settings_page_common, - mock_get_service_organisation, + mock_get_organisation, no_reply_to_email_addresses, no_letter_contact_blocks, single_sms_sender, @@ -4166,7 +4186,7 @@ def test_service_settings_when_inbound_number_is_not_set( service_one, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, + mock_get_organisation, single_sms_sender, mocker, mock_get_all_letter_branding, @@ -4299,7 +4319,7 @@ def test_updates_sms_prefixing( def test_select_organisation( platform_admin_client, service_one, - mock_get_service_organisation, + mock_get_organisation, mock_get_organisations ): response = platform_admin_client.get( @@ -4319,7 +4339,7 @@ def test_select_organisation( def test_select_organisation_shows_message_if_no_orgs( platform_admin_client, service_one, - mock_get_service_organisation, + mock_get_organisation, mocker ): mocker.patch('app.organisations_client.get_organisations', return_value=[]) @@ -4338,7 +4358,7 @@ def test_select_organisation_shows_message_if_no_orgs( def test_update_service_organisation( platform_admin_client, service_one, - mock_get_service_organisation, + mock_get_organisation, mock_get_organisations, mock_update_service_organisation, ): @@ -4357,7 +4377,7 @@ def test_update_service_organisation( def test_update_service_organisation_does_not_update_if_same_value( platform_admin_client, service_one, - mock_get_service_organisation, + mock_get_organisation, mock_get_organisations, mock_update_service_organisation, ): @@ -4401,10 +4421,6 @@ def test_show_branding_request_page_when_no_branding_is_set( ): service_one['{}_branding'.format(branding_type)] = None service_one['organisation_type'] = organisation_type - mocker.patch( - 'app.organisations_client.get_service_organisation', - return_value=None, - ) page = client_request.get( '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type @@ -4473,9 +4489,13 @@ def test_show_branding_request_page_when_no_branding_is_set_but_organisation_exi branding_type ): service_one['{}_branding'.format(branding_type)] = None - service_one['organisation_type'] = organisation_type mocker.patch( - 'app.organisations_client.get_service_organisation', + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + mocker.patch( + 'app.organisations_client.get_organisation', return_value=organisation_json(organisation_type=organisation_type), ) @@ -4517,9 +4537,13 @@ def test_show_branding_request_page_when_no_branding_is_set_but_organisation_exi branding_type ): service_one['{}_branding'.format(branding_type)] = None - service_one['organisation_type'] = organisation_type mocker.patch( - 'app.organisations_client.get_service_organisation', + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + mocker.patch( + 'app.organisations_client.get_organisation', return_value=organisation_json(organisation_type=organisation_type), ) @@ -4548,7 +4572,12 @@ def test_show_email_branding_request_page_when_email_branding_is_set( ): service_one['email_branding'] = sample_uuid() mocker.patch( - 'app.organisations_client.get_service_organisation', + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + mocker.patch( + 'app.organisations_client.get_organisation', return_value=organisation_json(), ) @@ -4578,7 +4607,12 @@ def test_show_letter_branding_request_page_when_letter_branding_is_set( ): service_one['letter_branding'] = sample_uuid() mocker.patch( - 'app.organisations_client.get_service_organisation', + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) + mocker.patch( + 'app.organisations_client.get_organisation', return_value=organisation_json(), ) @@ -4613,10 +4647,6 @@ def test_back_link_on_branding_request_page( back_link_url, branding_type, ): - mocker.patch( - 'app.organisations_client.get_service_organisation', - return_value=organisation_json(), - ) if from_template: page = client_request.get( '.branding_request', service_id=SERVICE_ONE_ID, branding_type=branding_type, from_template=from_template @@ -4641,14 +4671,19 @@ def test_show_branding_request_page_when_branding_is_same_as_org( branding_type ): service_one['{}_branding'.format(branding_type)] = sample_uuid() + mocker.patch( + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID, + ) if branding_type == 'email': mocker.patch( - 'app.organisations_client.get_service_organisation', + 'app.organisations_client.get_organisation', return_value=organisation_json(email_branding_id=service_one['email_branding']), ) else: mocker.patch( - 'app.organisations_client.get_service_organisation', + 'app.organisations_client.get_organisation', return_value=organisation_json(letter_branding_id=service_one['letter_branding']), ) @@ -4717,10 +4752,14 @@ def test_submit_email_branding_request( expected_organisation, ): service_one['email_branding'] = sample_uuid() - mocker.patch( - 'app.organisations_client.get_service_organisation', - return_value=organisation_json(name=org_name) if org_name else None, + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID if org_name else None, + ) + mocker.patch( + 'app.organisations_client.get_organisation', + return_value=organisation_json(name=org_name), ) zendesk = mocker.patch( @@ -4798,8 +4837,13 @@ def test_submit_letter_branding_request( service_one['letter_branding'] = sample_uuid() mocker.patch( - 'app.organisations_client.get_service_organisation', - return_value=organisation_json(name=org_name) if org_name else None, + 'app.models.service.Service.organisation_id', + new_callable=PropertyMock, + return_value=ORGANISATION_ID if org_name else None, + ) + mocker.patch( + 'app.organisations_client.get_organisation', + return_value=organisation_json(name=org_name), ) zendesk = mocker.patch( @@ -4853,7 +4897,6 @@ def test_submit_letter_branding_request_redirects_if_from_template_is_set( branding_type, ): - mocker.patch('app.organisations_client.get_service_organisation', return_value=None) mocker.patch('app.main.views.service_settings.zendesk_client.create_ticket', autospec=True) data = {'options': 'something_else', 'something_else': 'Homer Simpson'} @@ -4886,11 +4929,6 @@ def test_submit_branding_when_something_else_is_only_option( branding_type, current_branding, ): - mocker.patch( - 'app.organisations_client.get_service_organisation', - return_value=None, - ) - zendesk = mocker.patch( 'app.main.views.service_settings.zendesk_client.create_ticket', autospec=True, @@ -4921,7 +4959,7 @@ def test_service_settings_links_to_branding_request_page_for_letters( no_letter_contact_blocks, single_sms_sender, mock_get_service_settings_page_common, - mock_get_service_organisation, + mock_get_organisation, ): service_one["restricted"] is False service_one['permissions'].append('letter') diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py index 49d84ab07..8c8a426fb 100644 --- a/tests/app/models/test_service.py +++ b/tests/app/models/test_service.py @@ -3,6 +3,7 @@ import uuid from app.models.service import Service from app.models.user import User from tests import organisation_json +from tests.conftest import ORGANISATION_ID INV_PARENT_FOLDER_ID = '7e979e79-d970-43a5-ac69-b625a8d147b0' INV_CHILD_1_FOLDER_ID = '92ee1ee0-e4ee-4dcc-b1a7-a5da9ebcfa2b' @@ -176,18 +177,21 @@ def test_get_template_folders_shows_all_folders_when_user_id_not_passed_in( ] -def test_organisation_type_when_services_organisation_has_no_org_type(mocker, service_one, organisation_one): +def test_organisation_type_when_services_organisation_has_no_org_type(mocker, service_one): service = Service(service_one) - mocker.patch('app.organisations_client.get_service_organisation', return_value=organisation_one) + service._dict['organisation_id'] = ORGANISATION_ID + org = organisation_json(organisation_type=None) + mocker.patch('app.organisations_client.get_organisation', return_value=org) - assert not organisation_one['organisation_type'] + assert not org['organisation_type'] assert service.organisation_type == 'central' def test_organisation_type_when_service_and_its_org_both_have_an_org_type(mocker, service_one): # service_one has an organisation_type of 'central' service = Service(service_one) + service._dict['organisation'] = ORGANISATION_ID org = organisation_json(organisation_type='local') - mocker.patch('app.organisations_client.get_service_organisation', return_value=org) + mocker.patch('app.organisations_client.get_organisation', return_value=org) assert service.organisation_type == 'local' diff --git a/tests/conftest.py b/tests/conftest.py index fa31dc42f..2babc5429 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3113,7 +3113,7 @@ def mock_get_organisation(mocker): 'o1': 'Org 1', 'o2': 'Org 2', 'o3': 'Org 3', - }.get(org_id, 'Org 1'), + }.get(org_id, 'Test organisation'), ) return mocker.patch('app.organisations_client.get_organisation', side_effect=_get_organisation) @@ -3130,6 +3130,14 @@ def mock_get_organisation_by_domain(mocker): ) +@pytest.fixture(scope='function') +def mock_get_no_organisation_by_domain(mocker): + return mocker.patch( + 'app.organisations_client.get_organisation_by_domain', + return_value=None, + ) + + @pytest.fixture(scope='function') def mock_get_service_organisation( mocker,