From 4e721c95cedbcbed0c5353f8e23a3f21daf509d3 Mon Sep 17 00:00:00 2001 From: chrisw Date: Tue, 24 Oct 2017 15:37:44 +0100 Subject: [PATCH] Added Multiple SMS sender functionality --- app/main/forms.py | 14 ++ app/main/views/service_settings.py | 116 ++++++++- app/notify_client/inbound_number_client.py | 6 +- app/notify_client/service_api_client.py | 41 +++- app/templates/views/service-settings.html | 27 +-- .../service-settings/set-inbound-number.html | 35 +++ .../service-settings/sms-sender/add.html | 31 +++ .../service-settings/sms-sender/edit.html | 44 ++++ .../views/service-settings/sms-senders.html | 49 ++++ .../test_inbound_sms_setting.py | 179 ++++---------- .../test_service_setting_permissions.py | 3 +- tests/app/main/views/test_service_settings.py | 229 ++++++++++++++---- tests/conftest.py | 93 ++++++- 13 files changed, 638 insertions(+), 229 deletions(-) create mode 100644 app/templates/views/service-settings/set-inbound-number.html create mode 100644 app/templates/views/service-settings/sms-sender/add.html create mode 100644 app/templates/views/service-settings/sms-sender/edit.html create mode 100644 app/templates/views/service-settings/sms-senders.html diff --git a/app/main/forms.py b/app/main/forms.py index 33712c932..82c62288e 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -505,6 +505,7 @@ class ServiceSmsSender(Form): Length(max=11, message="Enter 11 characters or fewer") ] ) + is_default = BooleanField("Make this text message sender the default") def validate_sms_sender(self, field): if field.data and not re.match(r'^[a-zA-Z0-9\s]+$', field.data): @@ -674,6 +675,19 @@ class PasswordFieldShowHasContent(StringField): widget = widgets.PasswordInput(hide_value=False) +class ServiceInboundNumberForm(Form): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.inbound_number.choices = kwargs['inbound_number_choices'] + + inbound_number = RadioField( + "Select your inbound number", + validators=[ + DataRequired("Option must be selected") + ] + ) + + class ServiceInboundApiForm(Form): url = StringField("Inbound sms url", validators=[DataRequired(message='Can’t be empty'), diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 63eb21c31..2bc91599a 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -28,6 +28,7 @@ from app.main.forms import ( RenameServiceForm, RequestToGoLiveForm, ServiceReplyToEmailForm, + ServiceInboundNumberForm, ServiceSmsSender, ServiceLetterContactBlockForm, ServiceBrandingOrg, @@ -82,6 +83,11 @@ def service_settings(service_id): default_letter_contact_block = next( (Field(x['contact_block'], html='escape') for x in letter_contact_details if x['is_default']), "Not set" ) + sms_senders = service_api_client.get_sms_senders(service_id) + sms_sender_count = len(sms_senders) + default_sms_sender = next( + (Field(x['sms_sender'], html='escape') for x in sms_senders if x['is_default']), "None" + ) return render_template( 'views/service-settings.html', organisation=organisation, @@ -94,7 +100,9 @@ def service_settings(service_id): default_reply_to_email_address=default_reply_to_email_address, reply_to_email_address_count=reply_to_email_address_count, default_letter_contact_block=default_letter_contact_block, - letter_contact_details_count=letter_contact_details_count + letter_contact_details_count=letter_contact_details_count, + default_sms_sender=default_sms_sender, + sms_sender_count=sms_sender_count ) @@ -438,22 +446,34 @@ def service_set_sms_sender(service_id): form=form) -@main.route("/services//service-settings/set-inbound-number", methods=['GET']) +@main.route("/services//service-settings/set-inbound-number", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_settings', admin_override=True) def service_set_inbound_number(service_id): - switch_service_permissions(current_service['id'], 'inbound_sms') - set_inbound_sms = request.args.get('set_inbound_sms', False) - try: - if set_inbound_sms == 'True': - inbound_number_client.activate_inbound_sms_service(service_id) - return redirect(url_for('.service_settings', service_id=service_id)) - else: - inbound_number_client.deactivate_inbound_sms_permission(service_id=service_id) - return redirect(url_for('.service_set_sms_sender', service_id=service_id)) - except HTTPError as e: + available_inbound_numbers = inbound_number_client.get_available_inbound_sms_numbers() + service_has_inbound_number = inbound_number_client.get_inbound_sms_number_for_service(service_id)['data'] != {} + inbound_numbers_value_and_label = [ + (number['id'], number['number']) for number in available_inbound_numbers['data'] + ] + no_available_numbers = available_inbound_numbers['data'] == [] + form = ServiceInboundNumberForm( + inbound_number_choices=inbound_numbers_value_and_label + ) + if form.validate_on_submit(): + service_api_client.add_sms_sender( + current_service['id'], + sms_sender=form.inbound_number.data, + is_default=True, + inbound_number_id=form.inbound_number.data + ) switch_service_permissions(current_service['id'], 'inbound_sms') - raise e + return redirect(url_for('.service_settings', service_id=service_id)) + return render_template( + 'views/service-settings/set-inbound-number.html', + form=form, + no_available_numbers=no_available_numbers, + service_has_inbound_number=service_has_inbound_number + ) @main.route("/services//service-settings/set-sms", methods=['GET']) @@ -559,6 +579,76 @@ def service_edit_letter_contact(service_id, letter_contact_id): letter_contact_id=letter_contact_block['id']) +@main.route("/services//service-settings/sms-sender", methods=['GET']) +@login_required +@user_has_permissions('manage_settings', admin_override=True) +def service_sms_senders(service_id): + + def attach_hint(sender): + hints = [] + if sender['is_default']: + hints += ["default"] + if sender['inbound_number_id']: + hints += ["recieves replies"] + if hints: + sender['hint'] = "(" + " and ".join(hints) + ")" + + sms_senders = service_api_client.get_sms_senders(service_id) + + for sender in sms_senders: + attach_hint(sender) + + return render_template( + 'views/service-settings/sms-senders.html', + sms_senders=sms_senders + ) + + +@main.route("/services//service-settings/sms-sender/add", methods=['GET', 'POST']) +@login_required +@user_has_permissions('manage_settings', admin_override=True) +def service_add_sms_sender(service_id): + form = ServiceSmsSender() + sms_sender_count = len(service_api_client.get_sms_senders(service_id)) + first_sms_sender = sms_sender_count == 0 + if form.validate_on_submit(): + service_api_client.add_sms_sender( + current_service['id'], + sms_sender=form.sms_sender.data.replace('\r', '') or None, + is_default=first_sms_sender if first_sms_sender else form.is_default.data + ) + return redirect(url_for('.service_sms_senders', service_id=service_id)) + return render_template( + 'views/service-settings/sms-sender/add.html', + form=form, + first_sms_sender=first_sms_sender) + + +@main.route("/services//service-settings/sms-sender//edit", methods=['GET', 'POST']) +@login_required +@user_has_permissions('manage_settings', admin_override=True) +def service_edit_sms_sender(service_id, sms_sender_id): + sms_sender = service_api_client.get_sms_sender(service_id, sms_sender_id) + form = ServiceSmsSender() + form.sms_sender.data = sms_sender['sms_sender'] + is_inbound_number = True if sms_sender['inbound_number_id'] else False + if request.method == 'GET': + form.is_default.data = sms_sender['is_default'] + if form.validate_on_submit(): + service_api_client.update_sms_sender( + current_service['id'], + sms_sender_id=sms_sender_id, + sms_sender=form.sms_sender.data.replace('\r', ''), + is_default=True if sms_sender['is_default'] else form.is_default.data + ) + return redirect(url_for('.service_sms_senders', service_id=service_id)) + return render_template( + 'views/service-settings/sms-sender/edit.html', + form=form, + sms_sender_id=sms_sender['id'], + is_inbound_number=is_inbound_number) + + @main.route("/services//service-settings/set-letter-contact-block", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_settings', admin_override=True) diff --git a/app/notify_client/inbound_number_client.py b/app/notify_client/inbound_number_client.py index a12890974..662f93fea 100644 --- a/app/notify_client/inbound_number_client.py +++ b/app/notify_client/inbound_number_client.py @@ -11,6 +11,9 @@ class InboundNumberClient(NotifyAdminAPIClient): self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] self.api_key = app.config['ADMIN_CLIENT_SECRET'] + def get_available_inbound_sms_numbers(self): + return self.get(url='/inbound-number/available') + def get_all_inbound_sms_number_service(self): return self.get('/inbound-number') @@ -19,6 +22,3 @@ class InboundNumberClient(NotifyAdminAPIClient): def activate_inbound_sms_service(self, service_id): return self.post(url='/inbound-number/service/{}'.format(service_id), data={}) - - def deactivate_inbound_sms_permission(self, service_id): - return self.post(url='/inbound-number/service/{}/off'.format(service_id), data={}) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 7713ccb00..e374487d5 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -331,19 +331,10 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) def get_letter_contacts(self, service_id): - return self.get( - "/service/{}/letter-contact".format( - service_id - ) - ) + return self.get("/service/{}/letter-contact".format(service_id)) def get_letter_contact(self, service_id, letter_contact_id): - return self.get( - "/service/{}/letter-contact/{}".format( - service_id, - letter_contact_id - ) - ) + return self.get("/service/{}/letter-contact/{}".format(service_id, letter_contact_id)) def add_letter_contact(self, service_id, contact_block, is_default=False): return self.post( @@ -369,6 +360,34 @@ class ServiceAPIClient(NotifyAdminAPIClient): def get_aggregate_platform_stats(self, params_dict=None): return self.get("/service/platform-stats", params=params_dict) + def get_sms_senders(self, service_id): + return self.get( + "/service/{}/sms-sender".format(service_id) + ) + + def get_sms_sender(self, service_id, sms_sender_id): + return self.get( + "/service/{}/sms-sender/{}".format(service_id, sms_sender_id) + ) + + def add_sms_sender(self, service_id, sms_sender, is_default=False, inbound_number_id=None): + data = { + "sms_sender": sms_sender, + "is_default": is_default + } + if inbound_number_id: + data["inbound_number_id"] = inbound_number_id + return self.post("/service/{}/sms-sender".format(service_id), data=data) + + def update_sms_sender(self, service_id, sms_sender_id, sms_sender, is_default=False): + return self.post( + "/service/{}/sms-sender/{}".format(service_id, sms_sender_id), + data={ + "sms_sender": sms_sender, + "is_default": is_default + } + ) + class ServicesBrowsableItem(BrowsableItem): @property diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 6e7f63739..4e279f043 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -80,19 +80,16 @@ {% if 'sms' in current_service.permissions %} {% call row() %} - {{ text_field('Text message sender') }} - {% if can_receive_inbound %} - {{ text_field(inbound_number) }} - {% else %} - {{ text_field(current_service.sms_sender) }} - {% endif %} - {% if (current_user.has_permissions([], admin_override=True) or not can_receive_inbound) and not can_receive_inbound %} - {{ edit_field('Change', url_for('.service_set_sms_sender', service_id=current_service.id, set_inbound_sms=False)) }} - {% else %} - {{ text_field('') }} - {% endif %} - + {% call field(status='default' if default_sms_sender == "None" else '') %} + {{ default_sms_sender | string | nl2br | safe if default_sms_sender else 'None'}} + {% if sms_sender_count > 1 %} +
+ {{ '…and %d more' | format(sms_sender_count - 1) }} +
+ {% endif %} + {% endcall %} + {{ edit_field('Manage' if sms_sender_count else 'Change', url_for('.service_sms_senders', service_id=current_service.id)) }} {% endcall %} {% call row() %} @@ -250,11 +247,7 @@ {% if 'sms' in current_service.permissions %}
  • - {% if can_receive_inbound %} - - Stop inbound sms - - {% else %} + {% if not can_receive_inbound %} Allow inbound sms diff --git a/app/templates/views/service-settings/set-inbound-number.html b/app/templates/views/service-settings/set-inbound-number.html new file mode 100644 index 000000000..72a11b5b8 --- /dev/null +++ b/app/templates/views/service-settings/set-inbound-number.html @@ -0,0 +1,35 @@ +{% extends "withnav_template.html" %} +{% from "components/textbox.html" import textbox %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/radios.html" import radios%} + +{% block service_page_title %} + Set Inbound Number +{% endblock %} + +{% block maincolumn_content %} +

    Set Inbound Number

    + {% if service_has_inbound_number %} +

    This service already has an inbound number

    + {{ page_footer( + back_link=url_for('.service_settings', service_id=current_service.id), + back_link_text='Back to settings' + ) }} + {% elif no_available_numbers %} +

    No available inbound numbers

    + {{ page_footer( + back_link=url_for('.service_settings', service_id=current_service.id), + back_link_text='Back to settings' + ) }} + {% else %} +
    + {{ radios(form.inbound_number) }} + {{ page_footer( + 'Save', + back_link=url_for('.service_settings', service_id=current_service.id), + back_link_text='Back' + ) }} +
    + {% endif %} + +{% endblock %} \ No newline at end of file diff --git a/app/templates/views/service-settings/sms-sender/add.html b/app/templates/views/service-settings/sms-sender/add.html new file mode 100644 index 000000000..e670d67b7 --- /dev/null +++ b/app/templates/views/service-settings/sms-sender/add.html @@ -0,0 +1,31 @@ +{% extends "withnav_template.html" %} +{% from "components/textbox.html" import textbox %} +{% from "components/checkbox.html" import checkbox %} +{% from "components/page-footer.html" import page_footer %} + +{% block service_page_title %} + Add text message sender +{% endblock %} + +{% block maincolumn_content %} + +

    Add text message sender

    +
    + {{ textbox( + form.sms_sender, + width='1-4', + hint='Up to 11 characters, letters, numbers and spaces only' + ) }} + {% if not first_sms_sender %} +
    + {{ checkbox(form.is_default) }} +
    + {% endif %} + {{ page_footer( + 'Save', + back_link=url_for('.service_sms_senders', service_id=current_service.id), + back_link_text='Back' + ) }} +
    + +{% endblock %} \ No newline at end of file diff --git a/app/templates/views/service-settings/sms-sender/edit.html b/app/templates/views/service-settings/sms-sender/edit.html new file mode 100644 index 000000000..81857c2cd --- /dev/null +++ b/app/templates/views/service-settings/sms-sender/edit.html @@ -0,0 +1,44 @@ +{% extends "withnav_template.html" %} +{% from "components/textbox.html" import textbox %} +{% from "components/checkbox.html" import checkbox %} +{% from "components/page-footer.html" import page_footer %} + +{% block service_page_title %} + Edit text message sender +{% endblock %} + +{% block maincolumn_content %} + +

    + Edit text message sender +

    +
    + {% if is_inbound_number %} +

    + {{ form.sms_sender.data }} + This phone number receives replies and can’t be changed +

    + {% else %} + {{ textbox( + form.sms_sender, + width='1-4', + hint='Up to 11 characters, letters, numbers and spaces only' + ) }} + {% endif %} + {% if form.is_default.data %} +

    + This is currently your text message sender for {{ current_service.name }} +

    + {% else %} +
    + {{ checkbox(form.is_default) }} +
    + {% endif %} + {{ page_footer( + 'Save', + back_link=None if request.args.get('from_template') else url_for('.service_sms_senders', service_id=current_service.id), + back_link_text='Back' + ) }} +
    + +{% endblock %} \ No newline at end of file diff --git a/app/templates/views/service-settings/sms-senders.html b/app/templates/views/service-settings/sms-senders.html new file mode 100644 index 000000000..ee11c3e4c --- /dev/null +++ b/app/templates/views/service-settings/sms-senders.html @@ -0,0 +1,49 @@ +{% extends "withnav_template.html" %} +{% from "components/api-key.html" import api_key %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/table.html" import row_group, row, text_field, edit_field, field, boolean_field, list_table %} + +{% block service_page_title %} + Text message senders +{% endblock %} + +{% block maincolumn_content %} +
    +
    +

    + Text message senders +

    +
    +
    + Add text message sender +
    +
    +
    + {% if not sms_senders %} +
    + You haven’t added any sms senders yet +
    + {% endif %} + {% for item in sms_senders %} +
    +

    + {{ item.sms_sender }} + {% if item.hint %} +   + + {{ item.hint }} + + {% endif %} +

    + + {% if sms_senders|length > 1 %} + {{ api_key(item.id, thing="ID") }} + {% endif %} +
    + {% endfor %} +
    +{% endblock %} diff --git a/tests/app/main/views/service_settings/test_inbound_sms_setting.py b/tests/app/main/views/service_settings/test_inbound_sms_setting.py index 730436f38..768b2da5c 100644 --- a/tests/app/main/views/service_settings/test_inbound_sms_setting.py +++ b/tests/app/main/views/service_settings/test_inbound_sms_setting.py @@ -1,78 +1,64 @@ import app -import pytest -from bs4 import BeautifulSoup from flask import url_for -from notifications_python_client.errors import HTTPError +from tests.conftest import normalize_spaces -def test_set_text_message_sender( - logged_in_client, - mock_update_service, - service_one +def test_set_inbound_sms_sets_a_number_for_service( + logged_in_client, + mock_add_sms_sender, + multiple_available_inbound_numbers, + service_one, + fake_uuid, + mock_no_inbound_number_for_service, + mocker ): - data = {"sms_sender": "elevenchars"} - response = logged_in_client.post(url_for('main.service_set_sms_sender', service_id=service_one['id']), - data=data) - assert response.status_code == 302 - assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True) + mocker.patch('app.service_api_client.update_service_with_properties') + data = { + "inbound_number": "781d9c60-7a7e-46b7-9896-7b045b992fa5", + } - mock_update_service.assert_called_with( + response = logged_in_client.post( + url_for('main.service_set_inbound_number', service_id=service_one['id']), + data=data + ) + + assert response.status_code == 302 + mock_add_sms_sender.assert_called_once_with( service_one['id'], - sms_sender="elevenchars" + sms_sender="781d9c60-7a7e-46b7-9896-7b045b992fa5", + is_default=True, + inbound_number_id="781d9c60-7a7e-46b7-9896-7b045b992fa5" ) -def test_get_inbound_number_in_service_settings( - logged_in_client, - mock_update_service, - mock_get_letter_organisations, - single_reply_to_email_address, - single_letter_contact_block, +def test_set_inbound_sms_when_no_available_inbound_numbers( + client_request, service_one, + no_available_inbound_numbers, + mock_no_inbound_number_for_service, mocker ): - mocker_get_inbound_number_fun = mocker.patch( - 'app.inbound_number_client.get_inbound_sms_number_for_service', - return_value={'data': {'number': '077777777', 'id': 'some_uuid'}}) + page = client_request.get( + 'main.service_set_inbound_number', + service_id=service_one['id'] + ) - response = logged_in_client.get(url_for('main.service_settings', service_id=service_one['id'])) - assert response.status_code == 200 - mocker_get_inbound_number_fun.assert_called_once_with(service_one['id']) - - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - element = page.find('span', {"id": "077777777"}) - assert not element + assert normalize_spaces(page.select_one('main p').text) == "No available inbound numbers" -def test_allow_inbound_sms_sets_a_number_for_service( - logged_in_client, - service_one, - mocker +def test_set_inbound_sms_when_service_already_has_sms( + client_request, + service_one, + multiple_available_inbound_numbers, + mock_get_inbound_number_for_service, ): - mocker.patch('app.service_api_client.update_service_with_properties') - mock_activate_inbound_sms = mocker.patch('app.inbound_number_client.activate_inbound_sms_service') - response = logged_in_client.get(url_for('main.service_set_inbound_number', - service_id=service_one['id'], - set_inbound_sms=True)) + page = client_request.get( + 'main.service_set_inbound_number', + service_id=service_one['id'] + ) - assert response.status_code == 302 - assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True) - mock_activate_inbound_sms.assert_called_once_with(service_one['id']) - - -def test_allow_inbound_sms_returns_400_if_no_numbers_available( - logged_in_client, - service_one, - mocker -): - mock_switch_service = mocker.patch('app.service_api_client.update_service_with_properties') - mock_activate_inbound = mocker.patch('app.inbound_number_client.activate_inbound_sms_service', - side_effect=HTTPError) - logged_in_client.get( - url_for('main.service_set_inbound_number', service_id=service_one['id'], set_inbound_sms='True')) - mock_activate_inbound.assert_called_once_with(service_one['id']) - assert mock_switch_service.call_count == 2 + assert normalize_spaces(page.select_one('main p').text) == "This service already has an inbound number" def test_set_text_message_sender_and_inbound_sms_permission_exists_return_403( @@ -92,84 +78,3 @@ def test_set_text_message_sender_and_inbound_sms_permission_exists_return_403( assert not update_service_mock.called assert app.current_service['permissions'] == ['inbound_sms'] - - -def test_turn_inbound_sms_off( - logged_in_client, - service_one, - mocker -): - service_one['permissions'] = ['inbound_sms'] - update_service_mock = mocker.patch('app.service_api_client.update_service', - return_value=service_one) - mock_deactivate_inbound = mocker.patch('app.inbound_number_client.deactivate_inbound_sms_permission') - - response = logged_in_client.get(url_for('main.service_set_inbound_number', service_id=service_one['id'], - set_inbound_sms=False)) - assert response.status_code == 302 - assert response.location == url_for('main.service_set_sms_sender', service_id=service_one['id'], _external=True) - - assert app.current_service['permissions'] == [] - mock_deactivate_inbound.assert_called_once_with(service_id=service_one['id']) - assert update_service_mock.called - - -def test_set_text_message_sender_and_not_inbound_sms( - logged_in_client, - service_one, - mocker -): - service_one['permissions'] = [] - update_service_mock = mocker.patch('app.service_api_client.update_service', - return_value=service_one) - - data = {"sms_sender": "elevenchars"} - response = logged_in_client.post(url_for('main.service_set_sms_sender', service_id=service_one['id'], - set_inbound_sms=False), - data=data) - assert response.status_code == 302 - assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True) - - update_service_mock.assert_called_with( - service_one['id'], - sms_sender="elevenchars" - ) - assert app.current_service['permissions'] == [] - - -@pytest.mark.parametrize('content, expected_error', [ - ("", "Can’t be empty"), - ("twelvecharss", "Enter 11 characters or fewer"), - (".", "Use letters and numbers only") -]) -def test_set_text_message_sender_validation( - logged_in_client, - mock_update_service, - service_one, - content, - expected_error, -): - response = logged_in_client.post(url_for( - 'main.service_set_sms_sender', - service_id=service_one['id']), - data={"sms_sender": content}, - follow_redirects=True - ) - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - - assert response.status_code == 200 - assert page.select(".error-message")[0].text.strip() == expected_error - assert not mock_update_service.called - - -def test_if_sms_sender_set_then_form_populated( - logged_in_client, - service_one, - mock_get_inbound_number_for_service -): - service_one['sms_sender'] = 'elevenchars' - response = logged_in_client.get(url_for('main.service_set_sms_sender', service_id=service_one['id'])) - - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.find(id='sms_sender')['value'] == 'elevenchars' diff --git a/tests/app/main/views/service_settings/test_service_setting_permissions.py b/tests/app/main/views/service_settings/test_service_setting_permissions.py index e5c60e3f4..0002e23bc 100644 --- a/tests/app/main/views/service_settings/test_service_setting_permissions.py +++ b/tests/app/main/views/service_settings/test_service_setting_permissions.py @@ -14,6 +14,7 @@ def get_service_settings_page( mock_get_letter_organisations, no_reply_to_email_addresses, no_letter_contact_blocks, + single_sms_sender, ): platform_admin_request = client_request_factory(logged_in_platform_admin_client) return functools.partial(platform_admin_request.get, 'main.service_settings', service_id=service_one['id']) @@ -35,7 +36,6 @@ def get_service_settings_page( ({'permissions': ['sms']}, '.service_switch_can_send_sms', {}, 'Stop sending sms'), ({'permissions': []}, '.service_switch_can_send_sms', {}, 'Allow to send sms'), - ({'permissions': ['sms', 'inbound_sms']}, '.service_set_inbound_number', {'set_inbound_sms': False}, 'Stop inbound sms'), # noqa ({'permissions': ['sms']}, '.service_set_inbound_number', {'set_inbound_sms': True}, 'Allow inbound sms'), ({'active': True}, '.archive_service', {}, 'Archive service'), @@ -87,6 +87,7 @@ def test_normal_user_doesnt_see_any_toggle_buttons( mock_get_letter_organisations, no_reply_to_email_addresses, no_letter_contact_blocks, + single_sms_sender, ): page = client_request.get('main.service_settings', service_id=service_one['id']) toggles = page.find('a', {'class': 'button'}) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index e07b27215..4ae91e081 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -14,15 +14,20 @@ from tests.conftest import ( active_user_with_permissions, platform_admin_user, normalize_spaces, - no_reply_to_email_addresses, multiple_reply_to_email_addresses, multiple_letter_contact_blocks, + multiple_sms_senders, + no_reply_to_email_addresses, no_letter_contact_blocks, + no_sms_senders, get_default_reply_to_email_address, get_non_default_reply_to_email_address, get_default_letter_contact_block, get_non_default_letter_contact_block, - SERVICE_ONE_ID, + get_default_sms_sender, + get_non_default_sms_sender, + get_inbound_number_sms_sender, + SERVICE_ONE_ID ) @@ -38,7 +43,7 @@ from tests.conftest import ( 'Label Value Action', 'Send text messages On Change', - 'Text message sender GOVUK Change', + 'Text message sender GOVUK Manage', 'International text messages Off Change', 'Receive text messages Off Change', @@ -57,7 +62,7 @@ from tests.conftest import ( 'Label Value Action', 'Send text messages On Change', - 'Text message sender GOVUK Change', + 'Text message sender GOVUK Manage', 'International text messages Off Change', 'Receive text messages Off Change', @@ -80,6 +85,7 @@ def test_should_show_overview( mock_get_letter_organisations, no_reply_to_email_addresses, no_letter_contact_blocks, + single_sms_sender, user, expected_rows, mock_get_inbound_number_for_service @@ -112,7 +118,7 @@ def test_should_show_overview( 'Label Value Action', 'Send text messages On Change', - 'Text message sender 0781239871', + 'Text message sender GOVUK Manage', 'International text messages On Change', 'Receive text messages On Change', 'API endpoint for received text messages Not set Change', @@ -131,7 +137,7 @@ def test_should_show_overview( 'Label Value Action', 'Send text messages On Change', - 'Text message sender GOVUK Change', + 'Text message sender GOVUK Manage', 'International text messages Off Change', 'Receive text messages Off Change', @@ -147,6 +153,7 @@ def test_should_show_overview_for_service_with_more_things_set( service_one, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, mock_get_organisation, mock_get_letter_organisations, mock_get_inbound_number_for_service, @@ -173,12 +180,13 @@ def test_service_settings_show_elided_api_url_if_needed( service_one, mock_get_letter_organisations, single_reply_to_email_address, + single_sms_sender, single_letter_contact_block, mocker, fake_uuid, url, elided_url, - mock_get_inbound_number_for_service, + mock_get_inbound_number_for_service ): service_one['permissions'] = ['sms', 'email', 'inbound_sms'] service_one['inbound_api'] = [fake_uuid] @@ -216,32 +224,13 @@ def test_if_cant_send_letters_then_cant_see_letter_contact_block( assert 'Letter contact block' not in response.get_data(as_text=True) -def test_if_can_receive_inbound_then_cant_change_sms_sender( - logged_in_client, - service_one, - mock_get_letter_organisations, - single_reply_to_email_address, - single_letter_contact_block, - mock_get_inbound_number_for_service -): - service_one['permissions'] = ['email', 'sms', 'inbound_sms'] - response = logged_in_client.get(url_for( - 'main.service_settings', service_id=service_one['id'] - )) - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - rows_as_text = [" ".join(row.text.split()) for row in page.find_all('tr')] - assert 'Text message sender 0781239871 Change' not in rows_as_text - assert url_for('main.service_request_to_go_live', service_id=service_one['id'], - set_inbound_sms=False) not in response.get_data(as_text=True) - assert '0781239871' in response.get_data(as_text=True) - - def test_letter_contact_block_shows_none_if_not_set( logged_in_client, service_one, mocker, single_reply_to_email_address, no_letter_contact_blocks, + single_sms_sender, mock_get_letter_organisations, mock_get_inbound_number_for_service ): @@ -261,6 +250,7 @@ def test_escapes_letter_contact_block( service_one, mocker, single_reply_to_email_address, + single_sms_sender, injected_letter_contact_block, mock_get_letter_organisations, mock_get_inbound_number_for_service @@ -312,6 +302,7 @@ def test_show_restricted_service( mock_get_letter_organisations, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, mock_get_inbound_number_for_service ): response = logged_in_client.get(url_for('main.service_settings', service_id=service_one['id'])) @@ -345,6 +336,7 @@ def test_show_live_service( mock_get_live_service, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, mock_get_letter_organisations, mock_get_inbound_number_for_service ): @@ -471,6 +463,7 @@ def test_should_redirect_after_request_to_go_live( active_user_with_permissions, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, mock_get_letter_organisations, mock_get_inbound_number_for_service, ): @@ -569,6 +562,7 @@ def test_route_permissions( service_one, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, mock_get_letter_organisations, route, mock_get_inbound_number_for_service @@ -628,6 +622,7 @@ def test_route_for_platform_admin( service_one, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, mock_get_letter_organisations, route, mock_get_inbound_number_for_service @@ -701,7 +696,8 @@ def test_and_more_hint_appears_on_settings_with_more_than_just_a_single_sender( mock_get_letter_organisations, mock_get_inbound_number_for_service, multiple_reply_to_email_addresses, - multiple_letter_contact_blocks + multiple_letter_contact_blocks, + multiple_sms_senders ): service_one['permissions'] = ['email', 'sms', 'letter'] @@ -710,22 +706,26 @@ def test_and_more_hint_appears_on_settings_with_more_than_just_a_single_sender( service_id=service_one['id'] ) - assert normalize_spaces( - page.select('tbody tr')[2].text - ) == "Email reply to addresses test@example.com …and 2 more Manage" - assert normalize_spaces( - page.select('tbody tr')[8].text - ) == "Sender addresses 1 Example Street …and 2 more Manage" + def get_row(page, index): + return normalize_spaces( + page.select('tbody tr')[index].text + ) + + assert get_row(page, 2) == "Email reply to addresses test@example.com …and 2 more Manage" + assert get_row(page, 4) == "Text message sender Example …and 2 more Manage" + assert get_row(page, 8) == "Sender addresses 1 Example Street …and 2 more Manage" @pytest.mark.parametrize('sender_list_page, expected_output', [ ('main.service_email_reply_to', 'test@example.com (default) Change'), ('main.service_letter_contact_details', '1 Example Street (default) Change'), + ('main.service_sms_senders', 'GOVUK (default) Change') ]) def test_api_ids_dont_show_on_option_pages_with_a_single_sender( client_request, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, sender_list_page, expected_output ): @@ -758,6 +758,12 @@ def test_api_ids_dont_show_on_option_pages_with_a_single_sender( '1 Example Street (default) Change 1234', '2 Example Street Change 5678', '3 Example Street Change 9457' + ), ( + 'main.service_sms_senders', + multiple_sms_senders, + 'Example (default and recieves replies) Change 1234', + 'Example 2 Change 5678', + 'Example 3 Change 9457' ), ] ) @@ -796,6 +802,11 @@ def test_default_option_shows_for_default_sender( no_letter_contact_blocks, 'You haven’t added any letter contact details yet' ), + ( + 'main.service_sms_senders', + no_sms_senders, + 'You haven’t added any sms senders yet' + ), ]) def test_no_senders_message_shows( client_request, @@ -822,7 +833,7 @@ def test_no_senders_message_shows( ('testtest', 'Enter a valid email address'), ('test@hello.com', 'Enter a government email address. If you think you should have access contact us') ]) -def test_incorrect_reply_to_email_address( +def test_incorrect_reply_to_email_address_input( reply_to_input, expected_error, client_request, @@ -842,7 +853,7 @@ def test_incorrect_reply_to_email_address( ('', 'Can’t be empty'), ('1 \n 2 \n 3 \n 4 \n 5 \n 6 \n 7 \n 8 \n 9 \n 0 \n a', 'Contains 11 lines, maximum is 10') ]) -def test_incorrect_letter_contact_block( +def test_incorrect_letter_contact_block_input( contact_block_input, expected_error, client_request, @@ -858,6 +869,26 @@ def test_incorrect_letter_contact_block( assert normalize_spaces(page.select_one('.error-message').text) == expected_error +@pytest.mark.parametrize('sms_sender_input, expected_error', [ + ('', 'Can’t be empty'), + ('abcdefghijkhgkg', 'Enter 11 characters or fewer') +]) +def test_incorrect_sms_sender_input( + sms_sender_input, + expected_error, + client_request, + no_sms_senders +): + page = client_request.post( + 'main.service_add_sms_sender', + service_id=SERVICE_ONE_ID, + _data={'sms_sender': sms_sender_input}, + _expected_status=200 + ) + + assert normalize_spaces(page.select_one('.error-message').text) == expected_error + + @pytest.mark.parametrize('fixture, data, api_default_args', [ (no_reply_to_email_addresses, {}, True), (multiple_reply_to_email_addresses, {}, False), @@ -914,6 +945,34 @@ def test_add_letter_contact( ) +@pytest.mark.parametrize('fixture, data, api_default_args', [ + (no_sms_senders, {}, True), + (multiple_sms_senders, {}, False), + (multiple_sms_senders, {"is_default": "y"}, True) +]) +def test_add_sms_sender( + fixture, + data, + api_default_args, + mocker, + client_request, + mock_add_sms_sender +): + fixture(mocker) + data['sms_sender'] = "Example" + client_request.post( + 'main.service_add_sms_sender', + service_id=SERVICE_ONE_ID, + _data=data + ) + + mock_add_sms_sender.assert_called_once_with( + SERVICE_ONE_ID, + sms_sender="Example", + is_default=api_default_args + ) + + @pytest.mark.parametrize('sender_page, fixture, checkbox_present', [ ('main.service_add_email_reply_to', no_reply_to_email_addresses, False), ('main.service_add_email_reply_to', multiple_reply_to_email_addresses, True), @@ -1000,6 +1059,37 @@ def test_edit_letter_contact_block( ) +@pytest.mark.parametrize('fixture, data, api_default_args', [ + (get_default_sms_sender, {"is_default": "y"}, True), + (get_default_sms_sender, {}, True), + (get_non_default_sms_sender, {}, False), + (get_non_default_sms_sender, {"is_default": "y"}, True) +]) +def test_edit_sms_sender( + fixture, + data, + api_default_args, + mocker, + fake_uuid, + client_request, + mock_update_sms_sender +): + fixture(mocker) + client_request.post( + 'main.service_edit_sms_sender', + service_id=SERVICE_ONE_ID, + sms_sender_id=fake_uuid, + _data=data + ) + + mock_update_sms_sender.assert_called_once_with( + SERVICE_ONE_ID, + sms_sender_id=fake_uuid, + sms_sender="GOVUK", + is_default=api_default_args + ) + + @pytest.mark.parametrize('sender_page, fixture, default_message, params, checkbox_present', [ ( 'main.service_edit_email_reply_to', @@ -1028,6 +1118,20 @@ def test_edit_letter_contact_block( 'This is the default contact details for service one letters', 'letter_contact_id', True + ), + ( + 'main.service_edit_sms_sender', + get_default_sms_sender, + 'This is currently your text message sender for service one', + 'sms_sender_id', + False + ), + ( + 'main.service_edit_sms_sender', + get_non_default_sms_sender, + 'This is currently your text message sender for service one', + 'sms_sender_id', + True ) ]) def test_default_box_shows_on_non_default_sender_details_while_editing( @@ -1059,6 +1163,34 @@ def test_default_box_shows_on_non_default_sender_details_while_editing( ) +@pytest.mark.parametrize('fixture, hide_textbox, fixture_sender_id', [ + (get_inbound_number_sms_sender, True, '1234'), + (get_default_sms_sender, False, '1234'), +]) +def test_inbound_sms_sender_is_not_editable( + client_request, + service_one, + fake_uuid, + fixture, + hide_textbox, + fixture_sender_id, + mocker +): + fixture(mocker) + + page = client_request.get( + '.service_edit_sms_sender', + service_id=SERVICE_ONE_ID, + sms_sender_id=fixture_sender_id, + ) + + assert (page.select_one('main input[name="sms_sender"]') is None) == hide_textbox + if hide_textbox: + assert normalize_spaces( + page.select_one('form[method="post"] p').text + ) == "GOVUK This phone number receives replies and can’t be changed" + + def test_switch_service_to_research_mode( logged_in_platform_admin_client, platform_admin_user, @@ -1107,7 +1239,8 @@ def test_shows_research_mode_indicator( mock_get_letter_organisations, single_reply_to_email_address, single_letter_contact_block, - mock_get_inbound_number_for_service, + single_sms_sender, + mock_get_inbound_number_for_service ): service_one['research_mode'] = True mocker.patch('app.service_api_client.update_service_with_properties', return_value=service_one) @@ -1126,7 +1259,8 @@ def test_does_not_show_research_mode_indicator( mock_get_letter_organisations, single_reply_to_email_address, single_letter_contact_block, - mock_get_inbound_number_for_service, + single_sms_sender, + mock_get_inbound_number_for_service ): response = logged_in_client.get(url_for('main.service_settings', service_id=service_one['id'])) assert response.status_code == 200 @@ -1692,8 +1826,9 @@ def test_archive_service_prompts_user( mocker, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, mock_get_letter_organisations, - mock_get_inbound_number_for_service, + mock_get_inbound_number_for_service ): mocked_fn = mocker.patch('app.service_api_client.post') @@ -1710,8 +1845,9 @@ def test_cant_archive_inactive_service( service_one, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, mock_get_letter_organisations, - mock_get_inbound_number_for_service, + mock_get_inbound_number_for_service ): service_one['active'] = False @@ -1743,8 +1879,9 @@ def test_suspend_service_prompts_user( mocker, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, mock_get_letter_organisations, - mock_get_inbound_number_for_service, + mock_get_inbound_number_for_service ): mocked_fn = mocker.patch('app.service_api_client.post') @@ -1762,8 +1899,9 @@ def test_cant_suspend_inactive_service( service_one, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, mock_get_letter_organisations, - mock_get_inbound_number_for_service, + mock_get_inbound_number_for_service ): service_one['active'] = False @@ -1797,9 +1935,10 @@ def test_resume_service_prompts_user( service_one, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, mocker, mock_get_letter_organisations, - mock_get_inbound_number_for_service, + mock_get_inbound_number_for_service ): service_one['active'] = False mocked_fn = mocker.patch('app.service_api_client.post') @@ -1818,8 +1957,9 @@ def test_cant_resume_active_service( service_one, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, mock_get_letter_organisations, - mock_get_inbound_number_for_service, + mock_get_inbound_number_for_service ): response = logged_in_platform_admin_client.get(url_for('main.service_settings', service_id=service_one['id'])) @@ -1880,6 +2020,7 @@ def test_service_settings_when_inbound_number_is_not_set( service_one, single_reply_to_email_address, single_letter_contact_block, + single_sms_sender, mocker, mock_get_letter_organisations, ): diff --git a/tests/conftest.py b/tests/conftest.py index 38ad538a6..d3a2a1a7d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -280,6 +280,7 @@ def multiple_sms_senders(mocker): 'sms_sender': 'Example', 'is_default': True, 'created_at': datetime.utcnow(), + 'inbound_number_id': '1234', 'updated_at': None }, { 'id': '5678', @@ -287,6 +288,7 @@ def multiple_sms_senders(mocker): 'sms_sender': 'Example 2', 'is_default': False, 'created_at': datetime.utcnow(), + 'inbound_number_id': None, 'updated_at': None }, { 'id': '9457', @@ -294,6 +296,7 @@ def multiple_sms_senders(mocker): 'sms_sender': 'Example 3', 'is_default': False, 'created_at': datetime.utcnow(), + 'inbound_number_id': None, 'updated_at': None } ] @@ -316,9 +319,10 @@ def single_sms_sender(mocker): { 'id': '1234', 'service_id': service_id, - 'sms_sender': 'Example', + 'sms_sender': 'GOVUK', 'is_default': True, 'created_at': datetime.utcnow(), + 'inbound_number_id': None, 'updated_at': None } ] @@ -332,9 +336,10 @@ def get_default_sms_sender(mocker): return { 'id': '1234', 'service_id': service_id, - 'sms_sender': 'Example', + 'sms_sender': 'GOVUK', 'is_default': True, 'created_at': datetime.utcnow(), + 'inbound_number_id': None, 'updated_at': None } @@ -347,15 +352,90 @@ def get_non_default_sms_sender(mocker): return { 'id': '1234', 'service_id': service_id, - 'service_id': service_id, + 'sms_sender': 'GOVUK', 'is_default': False, 'created_at': datetime.utcnow(), + 'inbound_number_id': None, 'updated_at': None } return mocker.patch('app.service_api_client.get_sms_sender', side_effect=_get) +@pytest.fixture(scope='function') +def get_inbound_number_sms_sender(mocker): + def _get(service_id, sms_sender_id): + return { + 'id': '1234', + 'service_id': service_id, + 'sms_sender': 'GOVUK', + 'is_default': False, + 'created_at': datetime.utcnow(), + 'inbound_number_id': '1234', + 'updated_at': None + } + + return mocker.patch('app.service_api_client.get_sms_sender', side_effect=_get) + + +@pytest.fixture(scope='function') +def mock_add_sms_sender(mocker): + def _add_sms_sender(service_id, sms_sender, is_default=False, inbound_number_id=None): + return + + return mocker.patch('app.service_api_client.add_sms_sender', side_effect=_add_sms_sender) + + +@pytest.fixture(scope='function') +def mock_update_sms_sender(mocker): + def _update_sms_sender(service_id, sms_sender_id, sms_sender, is_default=False): + return + + return mocker.patch('app.service_api_client.update_sms_sender', side_effect=_update_sms_sender) + + +@pytest.fixture(scope='function') +def multiple_available_inbound_numbers(mocker): + def _get(): + return {'data': [ + { + 'active': True, + 'created_at': '2017-10-18T16:57:14.154185Z', + 'id': '781d9c60-7a7e-46b7-9896-7b045b992fa7', + 'number': '0712121214', + 'provider': 'mmg', + 'service': None, + 'updated_at': None + }, { + 'active': True, + 'created_at': '2017-10-18T16:57:22.585806Z', + 'id': '781d9c60-7a7e-46b7-9896-7b045b992fa5', + 'number': '0712121215', + 'provider': 'mmg', + 'service': None, + 'updated_at': None + }, { + 'active': True, + 'created_at': '2017-10-18T16:57:38.585806Z', + 'id': '781d9c61-7a7e-46b7-9896-7b045b992fa5', + 'number': '0712121216', + 'provider': 'mmg', + 'service': None, + 'updated_at': None + } + ]} + + return mocker.patch('app.inbound_number_client.get_available_inbound_sms_numbers', side_effect=_get) + + +@pytest.fixture(scope='function') +def no_available_inbound_numbers(mocker): + def _get(): + return {'data': []} + + return mocker.patch('app.inbound_number_client.get_available_inbound_sms_numbers', side_effect=_get) + + @pytest.fixture(scope='function') def fake_uuid(): return sample_uuid() @@ -1638,6 +1718,13 @@ def mock_get_inbound_number_for_service(mocker): return_value={'data': {'number': '0781239871'}}) +@pytest.fixture(scope='function') +def mock_no_inbound_number_for_service(mocker): + return mocker.patch( + 'app.inbound_number_client.get_inbound_sms_number_for_service', + return_value={'data': {}}) + + @pytest.fixture(scope='function') def mock_has_permissions(mocker): def _has_permission(permissions=None, any_=False, admin_override=False):