Added Multiple SMS sender functionality

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'),

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_id>/service-settings/set-inbound-number", methods=['GET'])
+@main.route("/services/<service_id>/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')
+    available_inbound_numbers = inbound_number_client.get_available_inbound_sms_numbers()
-    set_inbound_sms = request.args.get('set_inbound_sms', False)
+    service_has_inbound_number = inbound_number_client.get_inbound_sms_number_for_service(service_id)['data'] != {}
-    try:
+    inbound_numbers_value_and_label = [
-        if set_inbound_sms == 'True':
+        (number['id'], number['number']) for number in available_inbound_numbers['data']
-            inbound_number_client.activate_inbound_sms_service(service_id)
+    ]
-            return redirect(url_for('.service_settings', service_id=service_id))
+    no_available_numbers = available_inbound_numbers['data'] == []
-        else:
+    form = ServiceInboundNumberForm(
-            inbound_number_client.deactivate_inbound_sms_permission(service_id=service_id)
+        inbound_number_choices=inbound_numbers_value_and_label
-            return redirect(url_for('.service_set_sms_sender', service_id=service_id))
+    )
-    except HTTPError as e:
+    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_id>/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_id>/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_id>/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_id>/service-settings/sms-sender/<sms_sender_id>/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_id>/service-settings/set-letter-contact-block", methods=['GET', 'POST'])
 @login_required
 @user_has_permissions('manage_settings', admin_override=True)

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={})

app/notify_client/service_api_client.py

@@ -331,19 +331,10 @@ class ServiceAPIClient(NotifyAdminAPIClient):
         )
 
     def get_letter_contacts(self, service_id):
-        return self.get(
+        return self.get("/service/{}/letter-contact".format(service_id))
-            "/service/{}/letter-contact".format(
-                service_id
-            )
-        )
 
     def get_letter_contact(self, service_id, letter_contact_id):
-        return self.get(
+        return self.get("/service/{}/letter-contact/{}".format(service_id, letter_contact_id))
-            "/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

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 %}
+            {% call field(status='default' if default_sms_sender == "None" else '') %}
-               {{ text_field(inbound_number) }}
+              {{ default_sms_sender | string | nl2br | safe if default_sms_sender else 'None'}}
-            {% else %}
+              {% if sms_sender_count > 1 %}
-               {{ text_field(current_service.sms_sender) }}
+                <div class="hint">
-            {% endif %}
+                  {{ '…and %d more' | format(sms_sender_count - 1) }}
-            {% if (current_user.has_permissions([], admin_override=True) or not can_receive_inbound)  and not can_receive_inbound %}
+                </div>
-              {{ edit_field('Change', url_for('.service_set_sms_sender', service_id=current_service.id, set_inbound_sms=False)) }}
+              {% endif %}
-            {% else %}
+            {% endcall %}
-              {{ text_field('') }}
+            {{ edit_field('Manage' if sms_sender_count else 'Change', url_for('.service_sms_senders', service_id=current_service.id)) }}
-            {% endif %}
-
           {% endcall %}
 
           {% call row() %}
@@ -250,11 +247,7 @@
         </li>
         {% if 'sms' in current_service.permissions %}
           <li class="bottom-gutter">
-          {% if can_receive_inbound %}
+          {% if not can_receive_inbound %}
-            <a href="{{ url_for('.service_set_inbound_number', service_id=current_service.id, set_inbound_sms=False) }}" class="button">
-              Stop inbound sms
-            </a>
-          {% else %}
             <a href="{{ url_for('.service_set_inbound_number', service_id=current_service.id, set_inbound_sms=True) }}" class="button">
               Allow inbound sms
             </a>

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 %}
+    <h1 class="heading-large">Set Inbound Number</h1>
+    {% if service_has_inbound_number %}
+    <p> This service already has an inbound number </p>
+    {{ page_footer(
+	    back_link=url_for('.service_settings', service_id=current_service.id),
+	    back_link_text='Back to settings'
+	  ) }}
+    {% elif no_available_numbers %}
+    <p> No available inbound numbers </p>
+    {{ page_footer(
+	    back_link=url_for('.service_settings', service_id=current_service.id),
+	    back_link_text='Back to settings'
+	  ) }}
+    {% else %}
+    <form method="post">
+	    {{ radios(form.inbound_number) }}
+	    {{ page_footer(
+		    'Save',
+		    back_link=url_for('.service_settings', service_id=current_service.id),
+		    back_link_text='Back'
+	 	) }}
+	</form>
+	{% endif %}
+
+{% endblock %}

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 %}
+
+  <h1 class="heading-large">Add text message sender</h1>
+  <form method="post">
+    {{ textbox(
+      form.sms_sender,
+      width='1-4',
+      hint='Up to 11 characters, letters, numbers and spaces only'
+    ) }}
+    {% if not first_sms_sender %}
+      <div class="form-group">
+        {{ checkbox(form.is_default) }}
+      </div>
+    {% endif %}
+    {{ page_footer(
+      'Save',
+      back_link=url_for('.service_sms_senders', service_id=current_service.id),
+      back_link_text='Back'
+  ) }}
+  </form>
+
+{% endblock %}

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 %}
+
+  <h1 class="heading-large">
+    Edit text message sender
+  </h1>
+  <form method="post">
+    {% if is_inbound_number %}
+      <p>
+        <span class="bottom-gutter-1-3"> {{ form.sms_sender.data }} </span>
+        <span class="hint"> This phone number receives replies and can’t be changed </span>
+      </p>
+    {% else %}
+      {{ textbox(
+        form.sms_sender,
+        width='1-4',
+        hint='Up to 11 characters, letters, numbers and spaces only'
+      ) }}
+    {% endif %}
+    {% if form.is_default.data %}
+      <p class="form-group">
+        This is currently your text message sender for {{ current_service.name }}
+      </p>
+    {% else %}
+      <div class="form-group">
+        {{ checkbox(form.is_default) }}
+      </div>
+    {% 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'
+    ) }}
+  </form>
+
+{% endblock %}

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 %}
+  <div class="grid-row bottom-gutter">
+    <div class="column-two-thirds">
+      <h1 class="heading-large">
+        Text message senders
+      </h1>
+    </div>
+	 <div class="column-one-third">
+	   <a href="{{ url_for('.service_add_sms_sender', service_id=current_service.id) }}" class="button align-with-heading">Add text message sender</a>
+	 </div>
+  </div>
+  <div class="user-list">
+    {% if not sms_senders %}
+      <div class="user-list-item">
+        <span class="hint">You haven’t added any sms senders yet</span>
+      </div>
+    {% endif %} 
+    {% for item in sms_senders %}
+      <div class="user-list-item">
+        <h3>
+          <span class="heading-small">{{ item.sms_sender }}</span>
+          {% if item.hint %}
+            &ensp;
+            <span class="hint">
+              {{ item.hint }}
+            </span>
+          {% endif %}
+        </h3>
+        <ul class="tick-cross-list">
+          <li class="tick-cross-list-edit-link">
+            <a href="{{ url_for('.service_edit_sms_sender', service_id=current_service.id, sms_sender_id = item.id) }}">Change</a>
+          </li>
+        </ul>
+        {% if sms_senders|length  > 1 %}
+          {{ api_key(item.id, thing="ID") }}
+        {% endif %}
+      </div>
+    {% endfor %}
+  </div>
+{% endblock %}

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(
+def test_set_inbound_sms_sets_a_number_for_service(
-        logged_in_client,
+    logged_in_client,
-        mock_update_service,
+    mock_add_sms_sender,
-        service_one
+    multiple_available_inbound_numbers,
+    service_one,
+    fake_uuid,
+    mock_no_inbound_number_for_service,
+    mocker
 ):
-    data = {"sms_sender": "elevenchars"}
+    mocker.patch('app.service_api_client.update_service_with_properties')
-    response = logged_in_client.post(url_for('main.service_set_sms_sender', service_id=service_one['id']),
+    data = {
-                                     data=data)
+        "inbound_number": "781d9c60-7a7e-46b7-9896-7b045b992fa5",
-    assert response.status_code == 302
+    }
-    assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True)
 
-    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(
+def test_set_inbound_sms_when_no_available_inbound_numbers(
-        logged_in_client,
+        client_request,
-        mock_update_service,
-        mock_get_letter_organisations,
-        single_reply_to_email_address,
-        single_letter_contact_block,
         service_one,
+        no_available_inbound_numbers,
+        mock_no_inbound_number_for_service,
         mocker
 ):
-    mocker_get_inbound_number_fun = mocker.patch(
+    page = client_request.get(
-        'app.inbound_number_client.get_inbound_sms_number_for_service',
+        'main.service_set_inbound_number',
-        return_value={'data': {'number': '077777777', 'id': 'some_uuid'}})
+        service_id=service_one['id']
+    )
 
-    response = logged_in_client.get(url_for('main.service_settings', service_id=service_one['id']))
+    assert normalize_spaces(page.select_one('main p').text) == "No available inbound numbers"
-    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
 
 
-def test_allow_inbound_sms_sets_a_number_for_service(
+def test_set_inbound_sms_when_service_already_has_sms(
-        logged_in_client,
+    client_request,
-        service_one,
+    service_one,
-        mocker
+    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',
+    page = client_request.get(
-                                            service_id=service_one['id'],
+        'main.service_set_inbound_number',
-                                            set_inbound_sms=True))
+        service_id=service_one['id']
+    )
 
-    assert response.status_code == 302
+    assert normalize_spaces(page.select_one('main p').text) == "This service already has an inbound number"
-    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
 
 
 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'

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'})

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(
+    def get_row(page, index):
-        page.select('tbody tr')[2].text
+        return normalize_spaces(
-    ) == "Email reply to addresses test@example.com …and 2 more Manage"
+            page.select('tbody tr')[index].text
-    assert normalize_spaces(
+        )
-        page.select('tbody tr')[8].text
+
-    ) == "Sender addresses 1 Example Street …and 2 more Manage"
+    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,
 ):

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):