From 3565ffc33f7c17cb42ac3d01880a0bd44215d147 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 4 Apr 2019 11:23:40 +0100 Subject: [PATCH] Remove dependence on domains.yml from settings Settings looked at `domains.yml` when users were making go live requests or email branding requests. This will allow us to remove the `domains.yml` file, by using information about organisations that is now stored in the database instead. --- app/main/views/service_settings.py | 24 +++---- app/models/organisation.py | 12 ++-- app/models/service.py | 4 -- app/templates/views/service-settings.html | 2 +- tests/app/main/views/test_service_settings.py | 70 ++++++++++++------- tests/conftest.py | 14 +++- 6 files changed, 72 insertions(+), 54 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index ba20ccbb1..966ca1649 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -54,7 +54,6 @@ from app.main.forms import ( branding_options_dict, ) from app.utils import ( - AgreementInfo, email_safe, user_has_permissions, user_is_gov_user, @@ -180,7 +179,7 @@ def estimate_usage(service_id): @user_has_permissions('manage_service') def request_to_go_live(service_id): - agreement_signed = AgreementInfo.from_current_user().agreement_signed + agreement_signed = current_service.organisation.agreement_signed return render_template( 'views/service-settings/request-to-go-live.html', @@ -226,7 +225,7 @@ def submit_request_to_go_live(service_id): service_name=current_service.name, service_dashboard=url_for('main.service_dashboard', service_id=current_service.id, _external=True), organisation_type=str(current_service.organisation_type).title(), - agreement=AgreementInfo.from_current_user().as_human_readable, + agreement=current_service.organisation.as_human_readable(current_user.email_domain), checklist=current_service.go_live_checklist_completed_as_yes_no, volume_email=print_if_number(current_service.volume_email), volume_email_formatted=format_if_number(current_service.volume_email), @@ -237,7 +236,7 @@ def submit_request_to_go_live(service_id): research_consent='Yes' if current_service.consent_to_research else 'No', existing_live='Yes' if user_api_client.user_has_live_services(current_user) else 'No', service_id=current_service.id, - organisation=AgreementInfo.from_current_user().owner, + organisation=current_service.organisation.name, user_name=current_user.name, user_email=current_user.email_address, date=datetime.now(tz=pytz.timezone('Europe/London')).strftime('%d/%m/%Y'), @@ -245,7 +244,7 @@ def submit_request_to_go_live(service_id): ticket_type=zendesk_client.TYPE_QUESTION, user_email=current_user.email_address, user_name=current_user.name, - tags=get_request_to_go_live_tags(current_service, current_user), + tags=get_request_to_go_live_tags(current_service), ) flash('Thanks for your request to go live. We’ll get back to you within one working day.', 'default') @@ -985,7 +984,7 @@ def branding_request(service_id): '\nCurrent branding: {current_branding}' '\nBranding requested: {branding_requested}' ).format( - organisation=AgreementInfo.from_current_user().as_info_for_branding_request, + organisation=current_service.organisation.as_info_for_branding_request(current_user.email_domain), service_name=current_service.name, dashboard_url=url_for('main.service_dashboard', service_id=current_service.id, _external=True), current_branding=current_service.email_branding_name, @@ -1073,20 +1072,17 @@ def check_contact_details_type(contact_details): return 'phone_number' -def get_request_to_go_live_tags(service, user): - return list(_get_request_to_go_live_tags( - service, - AgreementInfo.from_user(user).agreement_signed, - )) +def get_request_to_go_live_tags(service): + return list(_get_request_to_go_live_tags(service)) -def _get_request_to_go_live_tags(service, agreement_signed): +def _get_request_to_go_live_tags(service): BASE = 'notify_request_to_go_live' yield BASE - if service.go_live_checklist_completed and agreement_signed: + if service.go_live_checklist_completed and service.organisation.agreement_signed: yield BASE + '_complete' return @@ -1094,7 +1090,7 @@ def _get_request_to_go_live_tags(service, agreement_signed): (True, ''), (not service.volumes, '_volumes'), (not service.go_live_checklist_completed, '_checklist'), - (not agreement_signed, '_mou'), + (not service.organisation.agreement_signed, '_mou'), (service.needs_to_add_email_reply_to_address, '_email_reply_to'), (not service.has_team_members, '_team_member'), (not service.has_templates, '_template_content'), diff --git a/app/models/organisation.py b/app/models/organisation.py index d3753a431..e3f827ff7 100644 --- a/app/models/organisation.py +++ b/app/models/organisation.py @@ -31,13 +31,12 @@ class Organisation(JSONModel): def crown_status(self): return self.crown - @property - def as_human_readable(self): + def as_human_readable(self, fallback_domain): if 'dwp.' in ''.join(self.domains): return 'DWP - Requires OED approval' if self.agreement_signed: return 'Yes, on behalf of {}'.format(self.name) - elif self.owner: + elif self.name: return '{} (organisation is {}, {})'.format( { False: 'No', @@ -51,11 +50,10 @@ class Organisation(JSONModel): }.get(self.crown_status), ) else: - return 'Can’t tell (domain is {})'.format(self._domain) + return 'Can’t tell (domain is {})'.format(fallback_domain) - @property - def as_info_for_branding_request(self): - return self.owner or 'Can’t tell (domain is {})'.format(self._domain) + def as_info_for_branding_request(self, fallback_domain): + return self.name or 'Can’t tell (domain is {})'.format(fallback_domain) @property def as_jinja_template(self): diff --git a/app/models/service.py b/app/models/service.py index 425fceb02..c28e6ff62 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -396,10 +396,6 @@ class Service(JSONModel): organisations_client.get_service_organisation(self.id) ) - @property - def organisation_name(self): - return self.organisation.name - @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/service-settings.html b/app/templates/views/service-settings.html index 34b725d04..0a18bde59 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -300,7 +300,7 @@ {% call row() %} {{ text_field('Organisation')}} - {{ optional_text_field(current_service.organisation_name) }} + {{ optional_text_field(current_service.organisation.name) }} {{ edit_field('Change', url_for('.link_service_to_organisation', service_id=current_service.id)) }} {% endcall %} {% call row() %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 847bc3536..6fcabf559 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1,4 +1,3 @@ -from collections import namedtuple from functools import partial from unittest.mock import ANY, PropertyMock, call from urllib.parse import parse_qs, urlparse @@ -31,6 +30,7 @@ from tests.conftest import ( get_non_default_letter_contact_block, get_non_default_reply_to_email_address, get_non_default_sms_sender, + mock_get_service_organisation, multiple_letter_contact_blocks, multiple_reply_to_email_addresses, multiple_sms_senders, @@ -101,7 +101,7 @@ def mock_get_service_settings_page_common( 'Label Value Action', 'Live Off Change', 'Count in list of live services Yes Change', - 'Organisation Org 1 Change', + 'Organisation Test Organisation Change', 'Organisation type Central Change', 'Free text message allowance 250,000 Change', 'Email branding GOV.UK Change', @@ -616,6 +616,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, volumes, consent_to_research, expected_estimated_volumes_item, @@ -675,6 +676,7 @@ def test_should_check_if_estimated_volumes_provided( def test_should_check_for_sending_things_right( client_request, mocker, + mock_get_service_organisation, single_sms_sender, count_of_users_with_manage_service, expected_user_checklist_item, @@ -685,7 +687,6 @@ def test_should_check_for_sending_things_right( reply_to_email_addresses, expected_reply_to_checklist_item, ): - def _templates_by_type(template_type): return { 'email': list(range(0, count_of_email_templates)), @@ -751,25 +752,22 @@ 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, single_sms_sender, checklist_completed, agreement_signed, expected_button, ): - - def _agreement_info(): - return namedtuple( - 'AgreementInfo', ['agreement_signed'] - )(agreement_signed=agreement_signed) - mocker.patch( 'app.models.service.Service.go_live_checklist_completed', new_callable=PropertyMock, return_value=checklist_completed, ) mocker.patch( - 'app.utils.AgreementInfo.from_current_user', - side_effect=_agreement_info, + 'app.models.organisation.Organisation.agreement_signed', + new_callable=PropertyMock, + return_value=agreement_signed, + create=True, ) for channel in ('email', 'sms', 'letter'): @@ -894,13 +892,13 @@ def test_should_check_for_sms_sender_on_go_live( client_request, service_one, mocker, + mock_get_service_organisation, organisation_type, count_of_sms_templates, sms_senders, expected_sms_sender_checklist_item, estimated_sms_volume, ): - service_one['organisation_type'] = organisation_type def _templates_by_type(template_type): @@ -953,18 +951,18 @@ def test_should_check_for_sms_sender_on_go_live( mock_get_sms_senders.assert_called_once_with(SERVICE_ONE_ID) -@pytest.mark.parametrize('email_address, expected_item', ( +@pytest.mark.parametrize('agreement_signed, expected_item', ( pytest.param( - 'test@unknown.gov.uk', + None, '', marks=pytest.mark.xfail(raises=IndexError) ), ( - 'test@education.gov.uk', + True, 'Sign our data sharing and financial agreement Completed', ), ( - 'test@aylesbury.gov.uk', + False, 'Sign our data sharing and financial agreement Not completed', ), )) @@ -972,7 +970,7 @@ def test_should_check_for_mou_on_request_to_go_live( client_request, service_one, mocker, - email_address, + agreement_signed, expected_item, ): mocker.patch( @@ -1000,9 +998,10 @@ def test_should_check_for_mou_on_request_to_go_live( return_value=None, ) - user = active_user_with_permissions(uuid4()) - user.email_address = email_address - client_request.login(user) + mock_get_service_organisation( + mocker, + agreement_signed=agreement_signed, + ) page = client_request.get( 'main.request_to_go_live', service_id=SERVICE_ONE_ID @@ -1017,6 +1016,7 @@ def test_non_gov_user_is_told_they_cant_go_live( client_request, api_nongov_user_active, mocker, + mock_get_service_organisation, ): mocker.patch( 'app.main.views.service_settings.user_api_client.get_count_of_users_with_permission', @@ -1287,7 +1287,6 @@ def test_should_redirect_after_request_to_go_live( active_user_with_permissions, single_reply_to_email_address, single_letter_contact_block, - mock_get_service_organisation, mock_get_organisations_and_services_for_user, single_sms_sender, mock_get_service_settings_page_common, @@ -1298,6 +1297,11 @@ def test_should_redirect_after_request_to_go_live( formatted_displayed_volumes, extra_tags, ): + mock_get_service_organisation( + mocker, + name=None, + agreement_signed=None, + ) for channel, volume in volumes: mocker.patch( 'app.models.service.Service.volume_{}'.format(channel), @@ -1516,6 +1520,11 @@ def test_ready_to_go_live( agreement_signed, expected_tags, ): + mock_get_service_organisation( + mocker, + agreement_signed=agreement_signed, + ) + for prop in { 'has_team_members', 'has_templates', @@ -1547,8 +1556,7 @@ def test_ready_to_go_live( }).go_live_checklist_completed_as_yes_no == expected_readyness assert list(app.main.views.service_settings._get_request_to_go_live_tags( - app.models.service.Service({'id': SERVICE_ONE_ID}), - agreement_signed, + app.models.service.Service({'id': SERVICE_ONE_ID}) )) == expected_tags @@ -3966,6 +3974,10 @@ def test_show_email_branding_request_page_when_email_branding_is_set( ('org_banner', 'Your logo on a colour'), pytest.param('foo', 'Nope', marks=pytest.mark.xfail(raises=AssertionError)), )) +@pytest.mark.parametrize('org_name, expected_organisation', ( + (None, 'Can’t tell (domain is user.gov.uk)'), + ('Test Organisation', 'Test Organisation'), +)) def test_submit_email_branding_request( client_request, mocker, @@ -3974,10 +3986,16 @@ def test_submit_email_branding_request( mock_get_service_settings_page_common, no_reply_to_email_addresses, no_letter_contact_blocks, - mock_get_service_organisation, single_sms_sender, + org_name, + expected_organisation, ): + mock_get_service_organisation( + mocker, + name=org_name, + ) + zendesk = mocker.patch( 'app.main.views.service_settings.zendesk_client.create_ticket', autospec=True, @@ -3993,14 +4011,14 @@ def test_submit_email_branding_request( zendesk.assert_called_once_with( message='\n'.join([ - 'Organisation: Can’t tell (domain is user.gov.uk)', + 'Organisation: {}', 'Service: service one', 'http://localhost/services/596364a0-858e-42c8-9062-a8fe822260eb', '', '---', 'Current branding: GOV.UK', 'Branding requested: {}', - ]).format(requested_branding), + ]).format(expected_organisation, requested_branding), subject='Email branding request - service one', ticket_type='question', user_email='test@user.gov.uk', diff --git a/tests/conftest.py b/tests/conftest.py index f29ffb2d2..21274f0ab 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3145,9 +3145,19 @@ def mock_get_organisation_by_domain( @pytest.fixture(scope='function') -def mock_get_service_organisation(mocker): +def mock_get_service_organisation( + mocker, + name=False, + crown=True, + agreement_signed=None, +): def _get_service_organisation(service_id): - return organisation_json('7aa5d4e9-4385-4488-a489-07812ba13383', 'Org 1') + return organisation_json( + '7aa5d4e9-4385-4488-a489-07812ba13383', + name, + crown=crown, + agreement_signed=agreement_signed, + ) return mocker.patch('app.organisations_client.get_service_organisation', side_effect=_get_service_organisation)