From f02ec309793f0868a947c393ec4eed977b3372d1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 13 Dec 2017 10:35:37 +0000 Subject: [PATCH 1/2] Make table labels match titles of callback pages This makes it consistent from page to page, and match the wording that Thom came up with. --- app/templates/views/api/callbacks.html | 4 ++-- .../views/api/callbacks/delivery-status-callback.html | 4 ++-- .../api/callbacks/received-text-messages-callback.html | 4 ++-- tests/app/main/views/test_api_keys.py | 8 +++++--- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/app/templates/views/api/callbacks.html b/app/templates/views/api/callbacks.html index 814bc50c4..6d502f67f 100644 --- a/app/templates/views/api/callbacks.html +++ b/app/templates/views/api/callbacks.html @@ -18,13 +18,13 @@ caption_visible=False ) %} {% call row() %} - {{ text_field('Delivery status callback URL') }} + {{ text_field('Callbacks for delivery receipts') }} {{ optional_text_field(received_text_messages_callback) }} {{ edit_field('Change', url_for('.delivery_status_callback', service_id=current_service.id)) }} {% endcall %} {% call row() %} - {{ text_field('Received text messages callback URL') }} + {{ text_field('Callbacks for received text messages') }} {{ optional_text_field(delivery_status_callback) }} {{ edit_field('Change', url_for('.received_text_messages_callback', service_id=current_service.id)) }} {% endcall %} diff --git a/app/templates/views/api/callbacks/delivery-status-callback.html b/app/templates/views/api/callbacks/delivery-status-callback.html index ab51f4cb8..a8c188c75 100644 --- a/app/templates/views/api/callbacks/delivery-status-callback.html +++ b/app/templates/views/api/callbacks/delivery-status-callback.html @@ -3,13 +3,13 @@ {% from "components/page-footer.html" import page_footer %} {% block service_page_title %} - Callback for delivery receipts + Callbacks for delivery receipts {% endblock %} {% block maincolumn_content %}
-

Callback for delivery receipts

+

Callbacks for delivery receipts

When you send an email or text message, we can tell you if Notify was able to deliver it. Check the callback documentation for more information. diff --git a/app/templates/views/api/callbacks/received-text-messages-callback.html b/app/templates/views/api/callbacks/received-text-messages-callback.html index ed9b4f13e..0ac44c3c6 100644 --- a/app/templates/views/api/callbacks/received-text-messages-callback.html +++ b/app/templates/views/api/callbacks/received-text-messages-callback.html @@ -3,13 +3,13 @@ {% from "components/page-footer.html" import page_footer %} {% block service_page_title %} - Callback for received text messages + Callbacks for received text messages {% endblock %} {% block maincolumn_content %} +

Callbacks for received text messages

-

Callback for received text messages

When you receive a text message in Notify, we can forward it to your system. Check the callback documentation for more information. diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index 1122ebefa..096426043 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -467,7 +467,7 @@ def test_callbacks_page_redirects_to_delivery_status_if_service_has_no_inbound_s _follow_redirects=True, ) - assert normalize_spaces(page.select_one('h1').text) == "Callback for delivery receipts" + assert normalize_spaces(page.select_one('h1').text) == "Callbacks for delivery receipts" @pytest.mark.parametrize('has_inbound_sms, expected_link', [ @@ -651,8 +651,10 @@ def test_callbacks_page_works_when_no_apis_set( page = client_request.get('main.api_callbacks', service_id=service_one['id'], _follow_redirects=True) - expected_rows = ['Delivery status callback URL Not set Change', - 'Received text messages callback URL Not set Change'] + expected_rows = [ + 'Callbacks for delivery receipts Not set Change', + 'Callbacks for received text messages Not set Change', + ] rows = page.select('tr') assert len(rows) == 3 for index, row in enumerate(expected_rows): From 3f01da05c75a8361e40a989f14853cb4c72776b7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 13 Dec 2017 11:42:41 +0000 Subject: [PATCH 2/2] Fix order of callbacks Delivery comes before inbound. The order of the URLs was jumbled in two places: - in the view function - in the Jinja template So as the user saw it the URLs were in the right order, because the double jumbling cancelled itself out. But it made the code _really_ confusing to read. --- app/main/views/api_keys.py | 2 +- app/templates/views/api/callbacks.html | 4 +-- tests/app/main/views/test_api_keys.py | 50 ++++++++++++++++++++------ 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index 4a86986a0..d8e6225f7 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -157,7 +157,7 @@ def api_callbacks(service_id): if 'inbound_sms' not in current_service['permissions']: return redirect(url_for('.delivery_status_callback', service_id=service_id)) - received_text_messages_callback, delivery_status_callback = get_apis() + delivery_status_callback, received_text_messages_callback = get_apis() return render_template( 'views/api/callbacks.html', diff --git a/app/templates/views/api/callbacks.html b/app/templates/views/api/callbacks.html index 6d502f67f..54a86bafe 100644 --- a/app/templates/views/api/callbacks.html +++ b/app/templates/views/api/callbacks.html @@ -19,13 +19,13 @@ ) %} {% call row() %} {{ text_field('Callbacks for delivery receipts') }} - {{ optional_text_field(received_text_messages_callback) }} + {{ optional_text_field(delivery_status_callback) }} {{ edit_field('Change', url_for('.delivery_status_callback', service_id=current_service.id)) }} {% endcall %} {% call row() %} {{ text_field('Callbacks for received text messages') }} - {{ optional_text_field(delivery_status_callback) }} + {{ optional_text_field(received_text_messages_callback) }} {{ edit_field('Change', url_for('.received_text_messages_callback', service_id=current_service.id)) }} {% endcall %} {% endcall %} diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index 096426043..b5e11f251 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -8,6 +8,7 @@ from unittest.mock import call from tests import validate_route_permission from tests.conftest import ( + fake_uuid, mock_get_service, mock_get_live_service, mock_get_service_with_letters, @@ -639,23 +640,52 @@ def test_update_delivery_status_and_receive_text_message_callbacks_without_chang assert mock_update_service_callback_api.called is False +@pytest.mark.parametrize('service_callback_api, delivery_url, expected_1st_table_row', [ + ( + None, {}, + 'Callbacks for delivery receipts Not set Change' + ), + ( + fake_uuid(), {'url': 'https://delivery.receipts'}, + 'Callbacks for delivery receipts https://delivery.receipts Change' + ), +]) +@pytest.mark.parametrize('inbound_api, inbound_url, expected_2nd_table_row', [ + ( + None, {}, + 'Callbacks for received text messages Not set Change' + ), + ( + fake_uuid(), {'url': 'https://inbound.sms'}, + 'Callbacks for received text messages https://inbound.sms Change' + ), +]) def test_callbacks_page_works_when_no_apis_set( - client_request, - service_one, - mocker + client_request, + service_one, + mocker, + service_callback_api, + delivery_url, + expected_1st_table_row, + inbound_api, + inbound_url, + expected_2nd_table_row, ): service_one['permissions'] = ['inbound_sms'] - mocker.patch('app.service_api_client.get_service_callback_api', side_effect={}) - mocker.patch('app.service_api_client.get_service_inbound_api', side_effect={}) + service_one['inbound_api'] = inbound_api + service_one['service_callback_api'] = service_callback_api + + mocker.patch('app.service_api_client.get_service_callback_api', return_value=delivery_url) + mocker.patch('app.service_api_client.get_service_inbound_api', return_value=inbound_url) page = client_request.get('main.api_callbacks', service_id=service_one['id'], _follow_redirects=True) expected_rows = [ - 'Callbacks for delivery receipts Not set Change', - 'Callbacks for received text messages Not set Change', + expected_1st_table_row, + expected_2nd_table_row, ] - rows = page.select('tr') - assert len(rows) == 3 + rows = page.select('tbody tr') + assert len(rows) == 2 for index, row in enumerate(expected_rows): - assert row == " ".join(rows[index + 1].text.split()) + assert row == normalize_spaces(rows[index].text)