Merge pull request #2838 from alphagov/fix-extra-go-live-task-logic

Fix the logic about showing extra task list items

app/models/service.py

@@ -200,6 +200,18 @@ class Service():
     def has_sms_templates(self):
         return len(self.get_templates('sms')) > 0
 
+    @property
+    def intending_to_send_email(self):
+        if self.volume_email is None:
+            return self.has_email_templates
+        return self.volume_email > 0
+
+    @property
+    def intending_to_send_sms(self):
+        if self.volume_sms is None:
+            return self.has_sms_templates
+        return self.volume_sms > 0
+
     @cached_property
     def email_reply_to_addresses(self):
         return service_api_client.get_reply_to_email_addresses(self.id)
@@ -226,7 +238,7 @@ class Service():
 
     @property
     def needs_to_add_email_reply_to_address(self):
-        return self.volume_email and not self.has_email_reply_to_address
+        return self.intending_to_send_email and not self.has_email_reply_to_address
 
     @property
     def shouldnt_use_govuk_as_sms_sender(self):
@@ -269,7 +281,7 @@ class Service():
     @property
     def needs_to_change_sms_sender(self):
         return all((
-            self.volume_sms,
+            self.intending_to_send_sms,
             self.shouldnt_use_govuk_as_sms_sender,
             self.sms_sender_is_govuk,
         ))

app/templates/views/service-settings/request-to-go-live.html

@@ -27,10 +27,7 @@
           'Add templates with examples of the content you plan to send',
           url_for('main.choose_template', service_id=current_service.id),
         ) }}
-        {% if (
-          current_service.has_email_templates
-          and (current_service.volume_email != 0)
-        ) %}
+        {% if current_service.intending_to_send_email %}
           {{ task_list_item(
             current_service.has_email_reply_to_address,
             'Add an email reply-to address',
@@ -38,9 +35,8 @@
           ) }}
         {% endif %}
         {% if (
-          current_service.has_sms_templates
+          current_service.intending_to_send_sms
           and current_service.shouldnt_use_govuk_as_sms_sender
-          and (current_service.volume_sms != 0)
         ) %}
           {{ task_list_item(
             not current_service.sms_sender_is_govuk,

tests/app/main/views/test_service_settings.py

@@ -570,11 +570,20 @@ def test_should_check_if_estimated_volumes_provided(
     (1, 'Add templates with examples of the content you plan to send Completed'),
     (2, 'Add templates with examples of the content you plan to send Completed'),
 ])
-@pytest.mark.parametrize('count_of_email_templates, reply_to_email_addresses, expected_reply_to_checklist_item', [
-    pytest.param(0, [], '', marks=pytest.mark.xfail(raises=IndexError)),
-    pytest.param(0, [{}], '', marks=pytest.mark.xfail(raises=IndexError)),
-    (1, [], 'Add an email reply-to address Not completed'),
-    (1, [{}], 'Add an email reply-to address Completed'),
+@pytest.mark.parametrize((
+    'volume_email,'
+    'count_of_email_templates,'
+    'reply_to_email_addresses,'
+    'expected_reply_to_checklist_item'
+), [
+    pytest.param(None, 0, [], '', marks=pytest.mark.xfail(raises=IndexError)),
+    pytest.param(0, 0, [], '', marks=pytest.mark.xfail(raises=IndexError)),
+    (None, 1, [], 'Add an email reply-to address Not completed'),
+    (None, 1, [{}], 'Add an email reply-to address Completed'),
+    (1, 1, [], 'Add an email reply-to address Not completed'),
+    (1, 1, [{}], 'Add an email reply-to address Completed'),
+    (1, 0, [], 'Add an email reply-to address Not completed'),
+    (1, 0, [{}], 'Add an email reply-to address Completed'),
 ])
 def test_should_check_for_sending_things_right(
     client_request,
@@ -584,6 +593,7 @@ def test_should_check_for_sending_things_right(
     expected_user_checklist_item,
     count_of_templates,
     expected_templates_checklist_item,
+    volume_email,
     count_of_email_templates,
     reply_to_email_addresses,
     expected_reply_to_checklist_item,
@@ -606,7 +616,7 @@ def test_should_check_for_sending_things_right(
         return_value=list(range(0, count_of_templates)),
     )
 
-    mock_get_templates = mocker.patch(
+    mocker.patch(
         'app.models.service.Service.get_templates',
         side_effect=_templates_by_type,
     )
@@ -616,12 +626,12 @@ def test_should_check_for_sending_things_right(
         return_value=reply_to_email_addresses
     )
 
-    for channel, volume in (('email', 1), ('sms', 0), ('letter', 1)):
+    for channel, volume in (('email', volume_email), ('sms', 0), ('letter', 1)):
         mocker.patch(
             'app.models.service.Service.volume_{}'.format(channel),
             create=True,
             new_callable=PropertyMock,
-            return_value=1,
+            return_value=volume,
         )
 
     page = client_request.get(
@@ -642,22 +652,20 @@ def test_should_check_for_sending_things_right(
     assert mock_templates.call_args_list == [
         call(),
     ]
-    assert mock_get_templates.call_args_list == [
-        call('email'),
-        call('sms'),
-    ]
 
     if count_of_email_templates:
         mock_get_reply_to_email_addresses.assert_called_once_with(SERVICE_ONE_ID)
 
 
-@pytest.mark.parametrize('estimated_sms_volume', (
-    pytest.param(None),
-    pytest.param(1),
-    pytest.param(0, marks=pytest.mark.xfail(raises=IndexError)),
-))
-@pytest.mark.parametrize('organisation_type,count_of_sms_templates, sms_senders, expected_sms_sender_checklist_item', [
+@pytest.mark.parametrize((
+    'estimated_sms_volume,'
+    'organisation_type,'
+    'count_of_sms_templates,'
+    'sms_senders,'
+    'expected_sms_sender_checklist_item'
+), [
     pytest.param(
+        0,
         'local',
         0,
         [],
@@ -665,6 +673,7 @@ def test_should_check_for_sending_things_right(
         marks=pytest.mark.xfail(raises=IndexError)
     ),
     pytest.param(
+        None,
         'local',
         0,
         [{'is_default': True, 'sms_sender': 'GOVUK'}],
@@ -672,6 +681,7 @@ def test_should_check_for_sending_things_right(
         marks=pytest.mark.xfail(raises=IndexError)
     ),
     pytest.param(
+        1,
         None,
         99,
         [{'is_default': True, 'sms_sender': 'GOVUK'}],
@@ -679,6 +689,15 @@ def test_should_check_for_sending_things_right(
         marks=pytest.mark.xfail(raises=IndexError)
     ),
     pytest.param(
+        None,
+        'central',
+        99,
+        [{'is_default': True, 'sms_sender': 'GOVUK'}],
+        '',
+        marks=pytest.mark.xfail(raises=IndexError)
+    ),
+    pytest.param(
+        1,
         'central',
         99,
         [{'is_default': True, 'sms_sender': 'GOVUK'}],
@@ -686,18 +705,28 @@ def test_should_check_for_sending_things_right(
         marks=pytest.mark.xfail(raises=IndexError)
     ),
     (
+        None,
         'local',
         1,
         [],
         'Change your text message sender name Not completed',
     ),
     (
+        1,
+        'local',
+        0,
+        [],
+        'Change your text message sender name Not completed',
+    ),
+    (
+        None,
         'local',
         1,
         [{'is_default': True, 'sms_sender': 'GOVUK'}],
         'Change your text message sender name Not completed',
     ),
     (
+        None,
         'local',
         1,
         [
@@ -707,6 +736,7 @@ def test_should_check_for_sending_things_right(
         'Change your text message sender name Completed',
     ),
     (
+        None,
         'nhs',
         1,
         [{'is_default': True, 'sms_sender': 'KUVOG'}],
@@ -741,7 +771,7 @@ def test_should_check_for_sms_sender_on_go_live(
         new_callable=PropertyMock,
         side_effect=partial(_templates_by_type, 'all'),
     )
-    mock_get_templates = mocker.patch(
+    mocker.patch(
         'app.models.service.Service.get_templates',
         side_effect=_templates_by_type,
     )
@@ -755,12 +785,13 @@ def test_should_check_for_sms_sender_on_go_live(
         return_value=[],
     )
 
-    mocker.patch(
-        'app.models.service.Service.volume_sms',
-        create=True,
-        new_callable=PropertyMock,
-        return_value=estimated_sms_volume,
-    )
+    for channel, volume in (('email', 0), ('sms', estimated_sms_volume)):
+        mocker.patch(
+            'app.models.service.Service.volume_{}'.format(channel),
+            create=True,
+            new_callable=PropertyMock,
+            return_value=volume,
+        )
 
     page = client_request.get(
         'main.request_to_go_live', service_id=SERVICE_ONE_ID
@@ -773,10 +804,6 @@ def test_should_check_for_sms_sender_on_go_live(
     assert mock_templates.call_args_list == [
         call(),
     ]
-    assert mock_get_templates.call_args_list == [
-        call('email'),
-        call('sms'),
-    ]
 
     mock_get_sms_senders.assert_called_once_with(SERVICE_ONE_ID)
 
@@ -820,6 +847,13 @@ def test_should_check_for_mou_on_request_to_go_live(
         'app.main.views.service_settings.service_api_client.get_reply_to_email_addresses',
         return_value=[],
     )
+    for channel in {'email', 'sms', 'letter'}:
+        mocker.patch(
+            'app.models.service.Service.volume_{}'.format(channel),
+            create=True,
+            new_callable=PropertyMock,
+            return_value=None,
+        )
 
     user = active_user_with_permissions(uuid4())
     user.email_address = email_address
@@ -1300,12 +1334,12 @@ def test_should_redirect_after_request_to_go_live(
         (  # Not done anything yet
             False,
             False,
-            True,
-            True,
+            False,
+            False,
+            False,
             False,
             True,
-            True,
-            0, None, 0,
+            None, None, None,
             'No',
             False,
             [
@@ -1351,14 +1385,17 @@ def test_ready_to_go_live(
             new_callable=PropertyMock
         ).return_value = locals()[prop]
 
-    mocker.patch(
-        'app.models.service.Service.__getattr__',
-        side_effect=lambda prop: {
-            'volume_email': volume_email,
-            'volume_sms': volume_sms,
-            'volume_letter': volume_letter,
-        }.get(prop)
-    )
+    for channel, volume in (
+        ('sms', volume_sms),
+        ('email', volume_email),
+        ('letter', volume_letter),
+    ):
+        mocker.patch(
+            'app.models.service.Service.volume_{}'.format(channel),
+            create=True,
+            new_callable=PropertyMock,
+            return_value=volume,
+        )
 
     assert app.models.service.Service({
         'id': SERVICE_ONE_ID