Merge pull request #1591 from gov-cjwaszczuk/master

Fixed broken edit functionality

app/main/forms.py

@@ -497,7 +497,7 @@ class ServiceReplyToEmailForm(Form):
     is_default = BooleanField("Make this email address the default")
 
 
-class ServiceSmsSender(Form):
+class ServiceSmsSenderForm(Form):
     sms_sender = StringField(
         'Text message sender',
         validators=[
@@ -512,6 +512,10 @@ class ServiceSmsSender(Form):
             raise ValidationError('Use letters and numbers only')
 
 
+class ServiceEditInboundNumberForm(Form):
+    is_default = BooleanField("Make this text message sender the default")
+
+
 class ServiceLetterContactBlockForm(Form):
     letter_contact_block = TextAreaField(
         validators=[

app/main/views/service_settings.py

@@ -29,7 +29,7 @@ from app.main.forms import (
     RequestToGoLiveForm,
     ServiceReplyToEmailForm,
     ServiceInboundNumberForm,
-    ServiceSmsSender,
+    ServiceSmsSenderForm,
     ServiceLetterContactBlockForm,
     ServiceBrandingOrg,
     LetterBranding,
@@ -37,6 +37,7 @@ from app.main.forms import (
     InternationalSMSForm,
     OrganisationTypeForm,
     FreeSMSAllowance,
+    ServiceEditInboundNumberForm,
 )
 from app import user_api_client, current_service, organisations_client, inbound_number_client
 from notifications_utils.formatters import formatted_list
@@ -428,7 +429,7 @@ def service_edit_email_reply_to(service_id, reply_to_email_id):
 @login_required
 @user_has_permissions('manage_settings', admin_override=True)
 def service_set_sms_sender(service_id):
-    form = ServiceSmsSender()
+    form = ServiceSmsSenderForm()
     if form.validate_on_submit():
         if 'inbound_sms' in current_service['permissions']:
             abort(403)
@@ -608,7 +609,7 @@ def service_sms_senders(service_id):
 @login_required
 @user_has_permissions('manage_settings', admin_override=True)
 def service_add_sms_sender(service_id):
-    form = ServiceSmsSender()
+    form = ServiceSmsSenderForm()
     sms_sender_count = len(service_api_client.get_sms_senders(service_id))
     first_sms_sender = sms_sender_count == 0
     if form.validate_on_submit():
@@ -629,24 +630,28 @@ def service_add_sms_sender(service_id):
 @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()
+    is_inbound_number = sms_sender['inbound_number_id']
-    form.sms_sender.data = sms_sender['sms_sender']
+    if is_inbound_number:
-    is_inbound_number = True if sms_sender['inbound_number_id'] else False
+        form = ServiceEditInboundNumberForm(is_default=sms_sender['is_default'])
-    if request.method == 'GET':
+    else:
-        form.is_default.data = sms_sender['is_default']
+        form = ServiceSmsSenderForm(**sms_sender)
+
     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', ''),
+            sms_sender=sms_sender['sms_sender'] if is_inbound_number else 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))
+
+    form.is_default.data = sms_sender['is_default']
     return render_template(
         'views/service-settings/sms-sender/edit.html',
         form=form,
-        sms_sender_id=sms_sender['id'],
+        sms_sender=sms_sender,
-        is_inbound_number=is_inbound_number)
+        inbound_number=is_inbound_number
+    )
 
 
 @main.route("/services/<service_id>/service-settings/set-letter-contact-block", methods=['GET', 'POST'])

app/templates/views/service-settings/sms-sender/edit.html

@@ -13,9 +13,9 @@
     Edit text message sender
   </h1>
   <form method="post">
-    {% if is_inbound_number %}
+    {% if inbound_number %}
       <p>
-        <span class="bottom-gutter-1-3"> {{ form.sms_sender.data }} </span>
+        <span class="bottom-gutter-1-3"> {{ sms_sender.sms_sender }} </span>
         <span class="hint"> This phone number receives replies and can’t be changed </span>
       </p>
     {% else %}

tests/app/main/test_validators.py

@@ -1,5 +1,5 @@
 import pytest
-from app.main.forms import RegisterUserForm, ServiceSmsSender
+from app.main.forms import RegisterUserForm, ServiceSmsSenderForm
 from app.main.validators import ValidGovEmail, NoCommasInPlaceHolders, OnlyGSMCharacters
 from wtforms import ValidationError
 from unittest.mock import Mock
@@ -184,7 +184,7 @@ def test_sms_sender_form_validation(
     client,
     mock_get_user_by_email,
 ):
-    form = ServiceSmsSender()
+    form = ServiceSmsSenderForm()
 
     form.sms_sender.data = 'elevenchars'
     form.validate()

tests/app/main/views/test_service_settings.py

@@ -1060,10 +1060,10 @@ 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, {"is_default": "y", "sms_sender": "test"}, True),
-    (get_default_sms_sender, {}, True),
+    (get_default_sms_sender, {"sms_sender": "test"}, True),
-    (get_non_default_sms_sender, {}, False),
+    (get_non_default_sms_sender, {"sms_sender": "test"}, False),
-    (get_non_default_sms_sender, {"is_default": "y"}, True)
+    (get_non_default_sms_sender, {"is_default": "y", "sms_sender": "test"}, True)
 ])
 def test_edit_sms_sender(
     fixture,
@@ -1085,7 +1085,7 @@ def test_edit_sms_sender(
     mock_update_sms_sender.assert_called_once_with(
         SERVICE_ONE_ID,
         sms_sender_id=fake_uuid,
-        sms_sender="GOVUK",
+        sms_sender="test",
         is_default=api_default_args
     )
 
@@ -1184,7 +1184,7 @@ def test_inbound_sms_sender_is_not_editable(
         sms_sender_id=fixture_sender_id,
     )
 
-    assert (page.select_one('main input[name="sms_sender"]') is None) == hide_textbox
+    assert bool(page.find('input', attrs={'name': "sms_sender"})) != hide_textbox
     if hide_textbox:
         assert normalize_spaces(
             page.select_one('form[method="post"] p').text