From c2dbc1934f93a40d2f762acc438d280414a909f4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 26 Apr 2018 17:23:44 +0100 Subject: [PATCH] Allow callbacks to be removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’ve had a user who’s said: > Seems configured callbacks cannot be removed once they’re set as the > fields have a presence check. Is that intentional? This means it’s not working as they expect. Rather than have to go and change stuff in the database for them, let’s make it work as they’d expect. Only lets you clear the form if you remove both the token and the URL. --- app/main/forms.py | 12 +++- app/main/views/api_keys.py | 30 ++++++--- app/notify_client/service_api_client.py | 12 ++++ tests/app/main/views/test_api_integration.py | 69 +++++++++++++++++++- tests/conftest.py | 32 +++++++++ 5 files changed, 144 insertions(+), 11 deletions(-) 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):