From 75f5829c1eb8cd2cde70e43771e1e7876b86f1ce Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 20 Apr 2017 14:11:51 +0100 Subject: [PATCH] Get organisations list from API rather than config Hard coding the organisations means this information is duplicated between the admin and the API, and could get out of sync. --- app/config.py | 5 ---- app/main/views/service_settings.py | 9 +++--- app/notify_client/organisations_client.py | 3 ++ tests/app/main/views/test_service_settings.py | 30 +++++++++++++++++-- tests/conftest.py | 13 ++++++++ 5 files changed, 48 insertions(+), 12 deletions(-) diff --git a/app/config.py b/app/config.py index 5899d6880..53eb1e8ac 100644 --- a/app/config.py +++ b/app/config.py @@ -83,11 +83,6 @@ class Config(object): r"assembly\.wales", ] - LETTER_BRANDING = [ - ('001', 'HM Government'), - ('500', 'Land Registry'), - ] - class Development(Config): DEBUG = True diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 3441c3c66..d19abab1c 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -36,6 +36,7 @@ from app import user_api_client, current_service, organisations_client @login_required @user_has_permissions('manage_settings', admin_override=True) def service_settings(service_id): + letter_branding_organisations = organisations_client.get_letter_organisations() if current_service['organisation']: organisation = organisations_client.get_organisation(current_service['organisation'])['organisation'] else: @@ -43,9 +44,9 @@ def service_settings(service_id): return render_template( 'views/service-settings.html', organisation=organisation, - letter_branding=dict( - current_app.config['LETTER_BRANDING'] - ).get(current_service.get('dvla_org_id', '001')) + letter_branding=letter_branding_organisations.get( + current_service.get('dvla_org_id', '001') + ) ) @@ -323,7 +324,7 @@ def service_set_branding_and_org(service_id): @user_has_permissions(admin_override=True) def set_letter_branding(service_id): - form = LetterBranding(choices=current_app.config['LETTER_BRANDING']) + form = LetterBranding(choices=organisations_client.get_letter_organisations().items()) if form.validate_on_submit(): service_api_client.update_service( diff --git a/app/notify_client/organisations_client.py b/app/notify_client/organisations_client.py index 58eb418f9..d18b9e66f 100644 --- a/app/notify_client/organisations_client.py +++ b/app/notify_client/organisations_client.py @@ -16,3 +16,6 @@ class OrganisationsClient(NotifyAdminAPIClient): def get_organisations(self): return self.get(url='/organisation')['organisations'] + + def get_letter_organisations(self): + return self.get(url='/dvla_organisations') diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 435ec1257..7b7095074 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -35,6 +35,7 @@ def test_should_show_overview( mocker, service_one, fake_uuid, + mock_get_letter_organisations, user, expected_rows, ): @@ -58,6 +59,7 @@ def test_should_show_overview_for_service_with_more_things_set( mocker, service_with_reply_to_addresses, mock_get_organisation, + mock_get_letter_organisations, ): client.login(active_user_with_permissions, mocker, service_with_reply_to_addresses) response = client.get(url_for( @@ -75,6 +77,7 @@ def test_should_show_overview_for_service_with_more_things_set( def test_if_cant_send_letters_then_cant_see_letter_contact_block( logged_in_client, service_one, + mock_get_letter_organisations, ): response = logged_in_client.get(url_for( 'main.service_settings', service_id=service_one['id'] @@ -85,7 +88,8 @@ def test_if_cant_send_letters_then_cant_see_letter_contact_block( def test_letter_contact_block_shows_None_if_not_set( logged_in_client, service_one, - mocker + mocker, + mock_get_letter_organisations, ): service_one['can_send_letters'] = True response = logged_in_client.get(url_for( @@ -102,6 +106,7 @@ def test_escapes_letter_contact_block( logged_in_client, service_one, mocker, + mock_get_letter_organisations, ): service_one['can_send_letters'] = True service_one['letter_contact_block'] = 'foo\nbar' @@ -148,6 +153,7 @@ def test_should_redirect_after_change_service_name( def test_show_restricted_service( logged_in_client, service_one, + mock_get_letter_organisations, ): response = logged_in_client.get(url_for('main.service_settings', service_id=service_one['id'])) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') @@ -176,7 +182,8 @@ def test_switch_service_to_live( def test_show_live_service( logged_in_client, service_one, - mock_get_live_service + mock_get_live_service, + mock_get_letter_organisations, ): response = logged_in_client.get(url_for('main.service_settings', service_id=service_one['id'])) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') @@ -296,6 +303,7 @@ def test_should_redirect_after_request_to_go_live( active_user_with_permissions, service_one, mocker, + mock_get_letter_organisations, ): mock_post = mocker.patch( 'app.main.views.feedback.requests.post', @@ -387,6 +395,7 @@ def test_route_permissions( client, api_user_active, service_one, + mock_get_letter_organisations, route, ): validate_route_permission( @@ -441,6 +450,7 @@ def test_route_for_platform_admin( client, platform_admin_user, service_one, + mock_get_letter_organisations, route, ): validate_route_permission(mocker, @@ -464,6 +474,7 @@ def test_route_for_platform_admin_update_service( client, platform_admin_user, service_one, + mock_get_letter_organisations, route, ): mocker.patch('app.service_api_client.archive_service') @@ -481,6 +492,7 @@ def test_set_reply_to_email_address( logged_in_client, mock_update_service, service_one, + mock_get_letter_organisations, ): data = {"email_address": "test@someservice.gov.uk"} response = logged_in_client.post(url_for('main.service_set_reply_to_email', service_id=service_one['id']), @@ -550,6 +562,7 @@ def test_shows_research_mode_indicator( logged_in_client, service_one, mocker, + mock_get_letter_organisations, ): service_one['research_mode'] = True mocker.patch('app.service_api_client.update_service_with_properties', return_value=service_one) @@ -565,6 +578,7 @@ def test_shows_research_mode_indicator( def test_does_not_show_research_mode_indicator( logged_in_client, service_one, + mock_get_letter_organisations, ): response = logged_in_client.get(url_for('main.service_settings', service_id=service_one['id'])) assert response.status_code == 200 @@ -578,6 +592,7 @@ def test_set_text_message_sender( logged_in_client, mock_update_service, service_one, + mock_get_letter_organisations, ): data = {"sms_sender": "elevenchars"} response = logged_in_client.post(url_for('main.service_set_sms_sender', service_id=service_one['id']), @@ -697,6 +712,7 @@ def test_set_letter_branding_platform_admin_only( def test_set_letter_branding_prepopulates( logged_in_platform_admin_client, service_one, + mock_get_letter_organisations, current_dvla_org_id, expected_selected, ): @@ -712,6 +728,7 @@ def test_set_letter_contact_block_saves( logged_in_platform_admin_client, service_one, mock_update_service, + mock_get_letter_organisations, ): response = logged_in_platform_admin_client.post( url_for('main.set_letter_branding', service_id=service_one['id']), @@ -725,7 +742,8 @@ def test_set_letter_contact_block_saves( def test_should_show_branding( logged_in_platform_admin_client, service_one, - mock_get_organisations + mock_get_organisations, + mock_get_letter_organisations, ): response = logged_in_platform_admin_client.get(url_for( 'main.service_set_branding_and_org', service_id=service_one['id'] @@ -845,6 +863,7 @@ def test_archive_service_prompts_user( logged_in_platform_admin_client, service_one, mocker, + mock_get_letter_organisations, ): mocked_fn = mocker.patch('app.service_api_client.post') @@ -859,6 +878,7 @@ def test_archive_service_prompts_user( def test_cant_archive_inactive_service( logged_in_platform_admin_client, service_one, + mock_get_letter_organisations, ): service_one['active'] = False @@ -887,6 +907,7 @@ def test_suspend_service_prompts_user( logged_in_platform_admin_client, service_one, mocker, + mock_get_letter_organisations, ): mocked_fn = mocker.patch('app.service_api_client.post') @@ -902,6 +923,7 @@ def test_suspend_service_prompts_user( def test_cant_suspend_inactive_service( logged_in_platform_admin_client, service_one, + mock_get_letter_organisations, ): service_one['active'] = False @@ -931,6 +953,7 @@ def test_resume_service_prompts_user( logged_in_platform_admin_client, service_one, mocker, + mock_get_letter_organisations, ): service_one['active'] = False mocked_fn = mocker.patch('app.service_api_client.post') @@ -947,6 +970,7 @@ def test_resume_service_prompts_user( def test_cant_resume_active_service( logged_in_platform_admin_client, service_one, + mock_get_letter_organisations, ): response = logged_in_platform_admin_client.get(url_for('main.service_settings', service_id=service_one['id'])) diff --git a/tests/conftest.py b/tests/conftest.py index 7805b6568..07ae87bc8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1357,6 +1357,19 @@ def mock_get_organisations(mocker): ) +@pytest.fixture(scope='function') +def mock_get_letter_organisations(mocker): + def _get_organisations(): + return { + '001': 'HM Government', + '500': 'Land Registry', + } + + return mocker.patch( + 'app.organisations_client.get_letter_organisations', side_effect=_get_organisations + ) + + @pytest.fixture(scope='function') def mock_get_organisation(mocker): def _get_organisation(id):