From 2a09429e1d4e36cdeb57bc1946cb68361c6e22f7 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Jun 2021 12:42:00 +0100 Subject: [PATCH 1/3] Remove duplication between no-radio-selected tests Previously the network selection case was tested here and also by 'test_post_service_set_broadcast_network_makes_you_choose'. I've renamed the test to be consistent and more specific. --- tests/app/main/views/test_service_settings.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 9946fff65..4c2b8eeaf 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -5962,29 +5962,21 @@ def test_post_service_set_broadcast_account_type_posts_data_to_api_and_redirects ) -@pytest.mark.parametrize('endpoint, extra_args, expected_error', ( - ('main.service_set_broadcast_channel', {}, 'Error: Select mode or channel'), - ('main.service_set_broadcast_network', {'broadcast_channel': 'government'}, 'Error: Select a mobile network'), -)) -def test_post_service_set_broadcast_account_type_shows_errors_if_no_radio_selected( +def test_post_service_set_broadcast_channel_makes_you_choose( platform_admin_client, mocker, - endpoint, - extra_args, - expected_error, ): set_service_broadcast_settings_mock = mocker.patch('app.service_api_client.set_service_broadcast_settings') mock_event_handler = mocker.patch('app.main.views.service_settings.create_broadcast_account_type_change_event') response = platform_admin_client.post( url_for( - endpoint, + 'main.service_set_broadcast_channel', service_id=SERVICE_ONE_ID, - **extra_args ) ) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert expected_error in page.find("span", {"class": "govuk-error-message"}).text + assert 'Error: Select mode or channel' in page.find("span", {"class": "govuk-error-message"}).text assert not set_service_broadcast_settings_mock.called assert not mock_event_handler.called From 9f3cd7332e334f2db0ce94d125b53df192734a2b Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Jun 2021 12:53:04 +0100 Subject: [PATCH 2/3] Add missing test for dodgy broadcast account types This moves the redundant assertions for the service not changing to where they're actually relevant, by comparing with the happy path [1]. [1]: https://github.com/alphagov/notifications-admin/blob/c5196fbf0790b7c0c566209c0d298c8d7190349c/tests/app/main/views/test_service_settings.py#L5858 --- tests/app/main/views/test_service_settings.py | 30 +++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 4c2b8eeaf..b40884c41 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -5900,7 +5900,7 @@ def test_post_service_set_broadcast_network_makes_you_choose( ]), ] ) -def test_post_service_set_broadcast_account_type_confirmation_page( +def test_post_service_confirm_broadcast_account_type_confirmation_page( client_request, platform_admin_user, value, @@ -5926,7 +5926,7 @@ def test_post_service_set_broadcast_account_type_confirmation_page( ("live-government", "live", "government", "all"), ] ) -def test_post_service_set_broadcast_account_type_posts_data_to_api_and_redirects( +def test_post_service_confirm_broadcast_account_type_posts_data_to_api_and_redirects( platform_admin_client, mocker, value, @@ -5962,13 +5962,35 @@ def test_post_service_set_broadcast_account_type_posts_data_to_api_and_redirects ) -def test_post_service_set_broadcast_channel_makes_you_choose( +@pytest.mark.parametrize('account_type', ( + 'foo-severe', + 'training-foo', + 'live-foo', + 'live-government-foo' +)) +def test_post_service_confirm_broadcast_account_type_errors_for_unknown_type( platform_admin_client, mocker, + account_type, ): set_service_broadcast_settings_mock = mocker.patch('app.service_api_client.set_service_broadcast_settings') mock_event_handler = mocker.patch('app.main.views.service_settings.create_broadcast_account_type_change_event') + response = platform_admin_client.post( + url_for( + 'main.service_confirm_broadcast_account_type', + service_id=SERVICE_ONE_ID, + account_type=account_type, + ) + ) + assert response.status_code == 404 + assert not set_service_broadcast_settings_mock.called + assert not mock_event_handler.called + + +def test_post_service_set_broadcast_channel_makes_you_choose( + platform_admin_client, +): response = platform_admin_client.post( url_for( 'main.service_set_broadcast_channel', @@ -5978,5 +6000,3 @@ def test_post_service_set_broadcast_channel_makes_you_choose( assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert 'Error: Select mode or channel' in page.find("span", {"class": "govuk-error-message"}).text - assert not set_service_broadcast_settings_mock.called - assert not mock_event_handler.called From e3cc16c936e1aff441ef979eb95d20c3efe92004 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Jun 2021 12:57:01 +0100 Subject: [PATCH 3/3] Remove redundant '_get' and '_post' in test names This is inconsistent with all the other tests in the same file, and one of them was incorrect ('_post' was testing a GET). I don't think we get any value from them, given the inconsistency. --- tests/app/main/views/test_service_settings.py | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index b40884c41..575c8e84a 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -5485,7 +5485,7 @@ def test_update_service_billing_details( ) -def test_get_service_set_broadcast_account_type( +def test_service_set_broadcast_channel( platform_admin_client, ): response = platform_admin_client.get( @@ -5515,7 +5515,7 @@ def test_get_service_set_broadcast_account_type( ) -def test_get_service_set_broadcast_account_type_has_no_radio_selected_for_non_broadcast_service( +def test_service_set_broadcast_channel_has_no_radio_selected_for_non_broadcast_service( platform_admin_client ): response = platform_admin_client.get( @@ -5562,7 +5562,7 @@ def test_get_service_set_broadcast_account_type_has_no_radio_selected_for_non_br ), ] ) -def test_get_service_set_broadcast_account_type_has_radio_selected_for_broadcast_service( +def test_service_set_broadcast_channel_has_radio_selected_for_broadcast_service( platform_admin_client, mocker, service_mode, @@ -5622,7 +5622,7 @@ def test_get_service_set_broadcast_account_type_has_radio_selected_for_broadcast ), ] ) -def test_get_service_set_broadcast_channel_redirects( +def test_service_set_broadcast_channel_redirects( client_request, platform_admin_user, mocker, @@ -5735,7 +5735,7 @@ def test_get_service_set_broadcast_channel_redirects( ), ] ) -def test_get_service_set_broadcast_network_has_radio_selected( +def test_service_set_broadcast_network_has_radio_selected( client_request, platform_admin_user, mocker, @@ -5783,7 +5783,7 @@ def test_get_service_set_broadcast_network_has_radio_selected( ('severe', 'vodafone', 'network'), ), ) -def test_post_service_set_broadcast_network( +def test_service_set_broadcast_network( client_request, platform_admin_user, broadcast_channel, @@ -5821,7 +5821,7 @@ def test_post_service_set_broadcast_network( ), ) @pytest.mark.parametrize('broadcast_channel', ['government', 'severe', 'test']) -def test_post_service_set_broadcast_network_makes_you_choose( +def test_service_set_broadcast_network_makes_you_choose( client_request, platform_admin_user, mocker, @@ -5900,7 +5900,7 @@ def test_post_service_set_broadcast_network_makes_you_choose( ]), ] ) -def test_post_service_confirm_broadcast_account_type_confirmation_page( +def test_service_confirm_broadcast_account_type_confirmation_page( client_request, platform_admin_user, value, @@ -5926,7 +5926,7 @@ def test_post_service_confirm_broadcast_account_type_confirmation_page( ("live-government", "live", "government", "all"), ] ) -def test_post_service_confirm_broadcast_account_type_posts_data_to_api_and_redirects( +def test_service_confirm_broadcast_account_type_posts_data_to_api_and_redirects( platform_admin_client, mocker, value, @@ -5968,7 +5968,7 @@ def test_post_service_confirm_broadcast_account_type_posts_data_to_api_and_redir 'live-foo', 'live-government-foo' )) -def test_post_service_confirm_broadcast_account_type_errors_for_unknown_type( +def test_service_confirm_broadcast_account_type_errors_for_unknown_type( platform_admin_client, mocker, account_type, @@ -5988,7 +5988,7 @@ def test_post_service_confirm_broadcast_account_type_errors_for_unknown_type( assert not mock_event_handler.called -def test_post_service_set_broadcast_channel_makes_you_choose( +def test_service_set_broadcast_channel_makes_you_choose( platform_admin_client, ): response = platform_admin_client.post(