diff --git a/app/main/forms.py b/app/main/forms.py index d88fdba51..03d15883b 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -826,7 +826,15 @@ class ServiceInboundNumberForm(StripWhitespaceForm): ) -class ServiceReceiveMessagesCallbackForm(StripWhitespaceForm): +class CallbackForm(StripWhitespaceForm): + + def validate(self): + return super().validate() or ( + self.url.data == '' and self.bearer_token.data == '' + ) + + +class ServiceReceiveMessagesCallbackForm(CallbackForm): url = StringField( "URL", validators=[DataRequired(message='Can’t be empty'), @@ -839,7 +847,7 @@ class ServiceReceiveMessagesCallbackForm(StripWhitespaceForm): ) -class ServiceDeliveryStatusCallbackForm(StripWhitespaceForm): +class ServiceDeliveryStatusCallbackForm(CallbackForm): url = StringField( "URL", validators=[DataRequired(message='Can’t be empty'), diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index 6a2a5c920..28293c8fc 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -208,9 +208,11 @@ def delivery_status_callback(service_id): ) if form.validate_on_submit(): - if delivery_status_callback: - if (delivery_status_callback.get('url') != form.url.data - or form.bearer_token.data != dummy_bearer_token): + if delivery_status_callback and form.url.data: + if ( + delivery_status_callback.get('url') != form.url.data or + form.bearer_token.data != dummy_bearer_token + ): service_api_client.update_service_callback_api( service_id, url=form.url.data, @@ -218,7 +220,12 @@ def delivery_status_callback(service_id): user_id=current_user.id, callback_api_id=delivery_status_callback.get('id') ) - else: + elif delivery_status_callback and not form.url.data: + service_api_client.delete_service_callback_api( + service_id, + delivery_status_callback['id'], + ) + elif form.url.data: service_api_client.create_service_callback_api( service_id, url=form.url.data, @@ -255,9 +262,11 @@ def received_text_messages_callback(service_id): ) if form.validate_on_submit(): - if received_text_messages_callback: - if (received_text_messages_callback.get('url') != form.url.data - or form.bearer_token.data != dummy_bearer_token): + if received_text_messages_callback and form.url.data: + if ( + received_text_messages_callback.get('url') != form.url.data or + form.bearer_token.data != dummy_bearer_token + ): service_api_client.update_service_inbound_api( service_id, url=form.url.data, @@ -265,7 +274,12 @@ def received_text_messages_callback(service_id): user_id=current_user.id, inbound_api_id=received_text_messages_callback.get('id') ) - else: + elif received_text_messages_callback and not form.url.data: + service_api_client.delete_service_inbound_api( + service_id, + received_text_messages_callback['id'], + ) + elif form.url.data: service_api_client.create_service_inbound_api( service_id, url=form.url.data, diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index af8a2481a..276872af3 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -332,6 +332,12 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) )['data'] + @cache.delete('service-{service_id}') + def delete_service_inbound_api(self, service_id, callback_api_id): + return self.delete("/service/{}/inbound-api/{}".format( + service_id, callback_api_id + )) + def get_reply_to_email_addresses(self, service_id): return self.get( "/service/{}/email-reply-to".format( @@ -463,6 +469,12 @@ class ServiceAPIClient(NotifyAdminAPIClient): data['bearer_token'] = bearer_token return self.post("/service/{}/delivery-receipt-api/{}".format(service_id, callback_api_id), data) + @cache.delete('service-{service_id}') + def delete_service_callback_api(self, service_id, callback_api_id): + return self.delete("/service/{}/delivery-receipt-api/{}".format( + service_id, callback_api_id + )) + @cache.delete('service-{service_id}') def create_service_callback_api(self, service_id, url, bearer_token, user_id): data = { diff --git a/tests/app/main/views/test_api_integration.py b/tests/app/main/views/test_api_integration.py index 148bdb9c5..4e0856c1d 100644 --- a/tests/app/main/views/test_api_integration.py +++ b/tests/app/main/views/test_api_integration.py @@ -10,6 +10,8 @@ from tests import validate_route_permission from tests.conftest import ( SERVICE_ONE_ID, fake_uuid, + mock_get_empty_service_callback_api, + mock_get_empty_service_inbound_api, mock_get_live_service, mock_get_notifications, mock_get_service, @@ -478,7 +480,7 @@ def test_should_validate_whitelist_items( ('main.received_text_messages_callback'), ]) @pytest.mark.parametrize('url, bearer_token, expected_errors', [ - ("", "", "Can’t be empty Can’t be empty"), + ("https://example.com", "", "Can’t be empty"), ("http://not_https.com", "1234567890", "Must be a valid https URL"), ("https://test.com", "123456789", "Must be at least 10 characters"), ]) @@ -509,6 +511,71 @@ def test_callback_forms_validation( assert error_msgs == expected_errors +@pytest.mark.parametrize('existing_values, callback_ids, delete_should_be_called', [ + (( + mock_get_valid_service_callback_api, + mock_get_valid_service_inbound_api, + ), [fake_uuid()], True), + (( + mock_get_empty_service_callback_api, + mock_get_empty_service_inbound_api, + ), [], False), +]) +@pytest.mark.parametrize('endpoint, expected_delete_url', [ + ( + 'main.delivery_status_callback', + '/service/{}/delivery-receipt-api/{}', + ), + ( + 'main.received_text_messages_callback', + '/service/{}/inbound-api/{}', + ), +]) +def test_callback_forms_can_be_cleared( + client_request, + service_one, + existing_values, + callback_ids, + delete_should_be_called, + endpoint, + expected_delete_url, + mocker, +): + + service_one['service_callback_api'] = callback_ids + service_one['inbound_api'] = callback_ids + mocked_delete = mocker.patch('app.service_api_client.delete') + for mock_get_callback in existing_values: + mock_get_callback(mocker) + + service_one['permissions'] = ['inbound_sms'] + + page = client_request.post( + endpoint, + service_id=service_one['id'], + _data={ + 'url': '', + 'bearer_token': '', + }, + _expected_redirect=url_for( + 'main.api_callbacks', + service_id=service_one['id'], + _external=True, + ) + ) + + assert not page.select(".error-message") + + if delete_should_be_called: + mocked_delete.assert_called_once_with( + expected_delete_url.format( + service_one['id'], callback_ids[0] + ) + ) + else: + assert mocked_delete.call_args_list == [] + + @pytest.mark.parametrize('has_inbound_sms, expected_link', [ (True, 'main.api_callbacks'), (False, 'main.delivery_status_callback'), diff --git a/tests/conftest.py b/tests/conftest.py index 2c457151d..f457d2c64 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2720,6 +2720,22 @@ def mock_get_valid_service_callback_api(mocker): return mocker.patch('app.service_api_client.get_service_callback_api', side_effect=_get) +@pytest.fixture(scope='function') +def mock_get_empty_service_inbound_api(mocker): + return mocker.patch( + 'app.service_api_client.get_service_inbound_api', + side_effect=lambda service_id, callback_api_id: None, + ) + + +@pytest.fixture(scope='function') +def mock_get_empty_service_callback_api(mocker): + return mocker.patch( + 'app.service_api_client.get_service_callback_api', + side_effect=lambda service_id, callback_api_id: None, + ) + + @pytest.fixture(scope='function') def mock_create_service_inbound_api(mocker): def _create_service_inbound_api(service_id, url, bearer_token, user_id): @@ -2728,6 +2744,14 @@ def mock_create_service_inbound_api(mocker): return mocker.patch('app.service_api_client.create_service_inbound_api', side_effect=_create_service_inbound_api) +@pytest.fixture(scope='function') +def mock_delete_service_inbound_api(mocker): + return mocker.patch( + 'app.service_api_client.delete_service_callback_api', + side_effect=lambda service_id: None + ) + + @pytest.fixture(scope='function') def mock_update_service_inbound_api(mocker): def _update_service_inbound_api(service_id, url, bearer_token, user_id, inbound_api_id): @@ -2744,6 +2768,14 @@ def mock_create_service_callback_api(mocker): return mocker.patch('app.service_api_client.create_service_callback_api', side_effect=_create_service_callback_api) +@pytest.fixture(scope='function') +def mock_delete_service_callback_api(mocker): + return mocker.patch( + 'app.service_api_client.delete_service_callback_api', + side_effect=lambda service_id: None + ) + + @pytest.fixture(scope='function') def mock_update_service_callback_api(mocker): def _update_service_callback_api(service_id, url, bearer_token, user_id, callback_api_id):