Merge pull request #1335 from alphagov/ken-use-new-service-permissions

Use new service permissions

app/main/views/send.py

@@ -245,7 +245,7 @@ def send_test_step(service_id, template_id, step_index):
         current_placeholder,
         dict_to_populate_from=get_normalised_send_test_values_from_session(),
         optional_placeholder=optional_placeholder,
-        allow_international_phone_numbers=current_service['can_send_international_sms'],
+        allow_international_phone_numbers='international_sms' in current_service['permissions'],
     )
 
     if form.validate_on_submit():
@@ -375,7 +375,7 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False):
             [user.name, user.mobile_number, user.email_address] for user in users
         ) if current_service['restricted'] else None,
         remaining_messages=remaining_messages,
-        international_sms=current_service['can_send_international_sms'],
+        international_sms='international_sms' in current_service['permissions'],
     )
 
     if request.args.get('from_test'):

app/main/views/service_settings.py

@@ -208,14 +208,26 @@ def service_switch_research_mode(service_id):
     return redirect(url_for('.service_settings', service_id=service_id))
 
 
+def switch_service_permissions(service_id, permission, sms_sender=None):
+    permissions = current_service['permissions'].copy()
+    if permission in permissions:
+        permissions.remove(permission)
+    else:
+        permissions.append(permission)
+    current_service['permissions'] = permissions
+
+    data = {'permissions': permissions}
+    if sms_sender:
+        data['sms_sender'] = sms_sender
+
+    service_api_client.update_service_with_properties(service_id, data)
+
+
 @main.route("/services/<service_id>/service-settings/can-send-letters")
 @login_required
 @user_has_permissions(admin_override=True)
 def service_switch_can_send_letters(service_id):
-    service_api_client.update_service_with_properties(
-        service_id,
-        {"can_send_letters": not current_service['can_send_letters']}
-    )
+    switch_service_permissions(service_id, 'letter')
     return redirect(url_for('.service_settings', service_id=service_id))
 
 
@@ -223,10 +235,7 @@ def service_switch_can_send_letters(service_id):
 @login_required
 @user_has_permissions(admin_override=True)
 def service_switch_can_send_international_sms(service_id):
-    service_api_client.update_service_with_properties(
-        service_id,
-        {"can_send_international_sms": not current_service['can_send_international_sms']}
-    )
+    switch_service_permissions(service_id, 'international_sms')
     return redirect(url_for('.service_settings', service_id=service_id))
 
 
@@ -295,16 +304,7 @@ def service_set_sms_sender(service_id):
     if form.validate_on_submit():
         set_inbound_sms = request.args.get('set_inbound_sms', False)
         if set_inbound_sms == 'True':
-            permissions = current_service['permissions']
-            if 'inbound_sms' in permissions:
-                permissions.remove('inbound_sms')
-            else:
-                permissions.append('inbound_sms')
-            service_api_client.update_service_with_properties(
-                current_service['id'],
-                {'permissions': permissions,
-                 'sms_sender': form.sms_sender.data or None}
-            )
+            switch_service_permissions(current_service['id'], 'inbound_sms', form.sms_sender.data)
         else:
             service_api_client.update_service(
                 current_service['id'],
@@ -350,7 +350,7 @@ def service_set_letters(service_id):
 @user_has_permissions('manage_settings', admin_override=True)
 def service_set_letter_contact_block(service_id):
 
-    if not current_service['can_send_letters']:
+    if 'letter' not in current_service['permissions']:
         abort(403)
 
     form = ServiceLetterContactBlock(letter_contact_block=current_service['letter_contact_block'])

app/main/views/templates.py

@@ -98,7 +98,7 @@ def choose_template(service_id, template_type='all'):
             ('All', 'all'),
             ('Text message', 'sms'),
             ('Email', 'email'),
-            ('Letter', 'letter') if current_service['can_send_letters'] else None,
+            ('Letter', 'letter') if 'letter' in current_service['permissions'] else None,
         ])
     ]
 
@@ -184,7 +184,7 @@ def view_template_version_preview(service_id, template_id, version, filetype):
 def add_template_by_type(service_id):
 
     form = ChooseTemplateType(
-        include_letters=current_service['can_send_letters']
+        include_letters='letter' in current_service['permissions']
     )
 
     if form.validate_on_submit():
@@ -220,7 +220,7 @@ def add_service_template(service_id, template_type):
 
     if template_type not in ['sms', 'email', 'letter']:
         abort(404)
-    if not current_service['can_send_letters'] and template_type == 'letter':
+    if 'letter' not in current_service['permissions'] and template_type == 'letter':
         abort(403)
 
     form = form_objects[template_type]()
@@ -448,5 +448,5 @@ def get_human_readable_delta(from_time, until_time):
 def should_show_template(template_type):
     return (
         template_type != 'letter' or
-        current_service['can_send_letters']
+        'letter' in current_service['permissions']
     )

app/notify_client/service_api_client.py

@@ -88,8 +88,6 @@ class ServiceAPIClient(NotifyAdminAPIClient):
             'email_from',
             'reply_to_email_address',
             'research_mode',
-            'can_send_letters',
-            'can_send_international_sms',
             'sms_sender',
             'created_by',
             'branding',

app/templates/views/dashboard/write-first-messages.html

@@ -2,7 +2,7 @@
 
 <nav>
   <a class="pill-separate-item" href="{{ url_for('.choose_template', service_id=current_service.id) }}">
-    {% if current_service.can_send_letters %}
+    {% if 'letter' in current_service.permissions %}
       Write an email, text message or letter
     {% else %}
       Write an email or text message

app/templates/views/service-settings.html

@@ -47,7 +47,7 @@
 
         {% call row() %}
           {{ text_field('International text messages') }}
-          {{ boolean_field(current_service.can_send_international_sms) }}
+          {{ boolean_field('international_sms' in current_service.permissions) }}
           {{ edit_field('Change', url_for('.service_set_international_sms', service_id=current_service.id)) }}
         {% endcall %}
 
@@ -70,11 +70,11 @@
 
         {% call row() %}
           {{ text_field('Letters') }}
-          {{ boolean_field(current_service.can_send_letters) }}
+          {{ boolean_field('letter' in current_service.permissions) }}
           {{ edit_field('Change', url_for('.service_set_letters', service_id=current_service.id)) }}
         {% endcall %}
 
-        {% if current_service.can_send_letters %}
+        {% if 'letter' in current_service.permissions %}
           {% call row() %}
             {{ text_field('Letter contact details') }}
             {% call field(status='' if current_service.letter_contact_block else 'default') %}
@@ -158,12 +158,12 @@
         </li>
         <li class="bottom-gutter">
           <a href="{{ url_for('.service_switch_can_send_letters', service_id=current_service.id) }}" class="button">
-            {{ 'Stop sending letters' if current_service.can_send_letters else 'Allow to send letters' }}
+            {{ 'Stop sending letters' if 'letter' in current_service.permissions else 'Allow to send letters' }}
           </a>
         </li>
         <li class="bottom-gutter">
           <a href="{{ url_for('.service_switch_can_send_international_sms', service_id=current_service.id) }}" class="button">
-            {{ 'Stop sending international sms' if current_service.can_send_international_sms else 'Allow to send international sms' }}
+            {{ 'Stop sending international sms' if 'international_sms' in current_service.permissions else 'Allow international sms' }}
           </a>
         </li>
         {% if current_service.active %}

app/templates/views/service-settings/set-international-sms.html

@@ -11,7 +11,7 @@
   <div class="grid-row">
     <div class="column-five-sixths">
       <h1 class="heading-large">International text messages</h1>
-      {% if current_service.can_send_international_sms %}
+      {% if 'international_sms' in current_service.permissions %}
         <p>
           Your service can send text messages to international phone numbers.
         </p>

app/templates/views/service-settings/set-letters.html

@@ -11,7 +11,7 @@
   <div class="grid-row">
     <div class="column-five-sixths">
       <h1 class="heading-large">Letters</h1>
-      {% if current_service.can_send_letters %}
+      {% if 'letter' in current_service.permissions %} 
         <p>
           Your service can send letters.
         </p>

app/templates/views/templates/choose.html

@@ -17,7 +17,7 @@
     {% if current_user.has_permissions(permissions=['manage_templates'], any_=True) %}
        <p class="bottom-gutter">
          You need a template before you can send
-         {% if current_service.can_send_letters %}
+         {% if 'letter' in current_service.permissions %}
            emails, text messages or letters
          {%- else -%}
            emails or text messages
@@ -27,7 +27,7 @@
     {% else %}
       <p>
         You need to ask your service manager to add templates before you can send
-        {% if current_service.can_send_letters %}
+        {% if 'letter' in current_service.permissions %}
           emails, text messages or letters
         {%- else -%}
           emails or text messages

tests/__init__.py

@@ -49,14 +49,12 @@ def service_json(
     reply_to_email_address=None,
     sms_sender='GOVUK',
     research_mode=False,
-    can_send_letters=False,
-    can_send_international_sms=False,
     organisation=None,
     branding='govuk',
     created_at=None,
     letter_contact_block=None,
-    permissions=None,
     inbound_api=None,
+    permissions=['email', 'sms'],
 ):
     if users is None:
         users = []
@@ -75,8 +73,6 @@ def service_json(
         'reply_to_email_address': reply_to_email_address,
         'sms_sender': sms_sender,
         'research_mode': research_mode,
-        'can_send_letters': can_send_letters,
-        'can_send_international_sms': can_send_international_sms,
         'organisation': organisation,
         'branding': branding,
         'created_at': created_at or str(datetime.utcnow()),

tests/app/main/views/test_letters.py

@@ -11,22 +11,24 @@ letters_urls = [
 
 
 @pytest.mark.parametrize('url', letters_urls)
-@pytest.mark.parametrize('can_send_letters, response_code', [
-    (True, 200),
-    (False, 403)
+@pytest.mark.parametrize('permissions, response_code', [
+    (['letter'], 200),
+    ([], 403)
 ])
 def test_letters_access_restricted(
     logged_in_platform_admin_client,
     mocker,
-    can_send_letters,
+    permissions,
     response_code,
     mock_get_service_templates,
     url,
+    service_one,
 ):
-    service = service_json(can_send_letters=can_send_letters)
-    mocker.patch('app.service_api_client.get_service', return_value={"data": service})
+    service_one['permissions'] = permissions
 
-    response = logged_in_platform_admin_client.get(url(service_id=service['id']))
+    mocker.patch('app.service_api_client.get_service', return_value={"data": service_one})
+
+    response = logged_in_platform_admin_client.get(url(service_id=service_one['id']))
 
     assert response.status_code == response_code
 
@@ -40,29 +42,30 @@ def test_letters_lets_in_without_permission(
     api_user_active,
     mock_get_service_templates,
     url,
+    service_one
 ):
-    service = service_json(can_send_letters=True)
-    mocker.patch('app.service_api_client.get_service', return_value={"data": service})
+    service_one['permissions'] = ['letter']
+    mocker.patch('app.service_api_client.get_service', return_value={"data": service_one})
 
     client.login(api_user_active)
-    response = client.get(url(service_id=service['id']))
+    response = client.get(url(service_id=service_one['id']))
 
     assert api_user_active.permissions == {}
     assert response.status_code == 200
 
 
-@pytest.mark.parametrize('can_send_letters, choices', [
-    (True, ['Email', 'Text message', 'Letter']),
-    (False, ['Email', 'Text message'])
+@pytest.mark.parametrize('permissions, choices', [
+    (['email', 'sms', 'letter'], ['Email', 'Text message', 'Letter']),
+    (['email', 'sms'], ['Email', 'Text message'])
 ])
 def test_given_option_to_add_letters_if_allowed(
     logged_in_client,
     service_one,
     mocker,
-    can_send_letters,
+    permissions,
     choices,
 ):
-    service_one['can_send_letters'] = can_send_letters
+    service_one['permissions'] = permissions
     mocker.patch('app.service_api_client.get_service', return_value={"data": service_one})
 
     response = logged_in_client.get(url_for('main.add_template_by_type', service_id=service_one['id']))

tests/app/main/views/test_send.py

@@ -853,7 +853,7 @@ def test_send_test_works_as_letter_preview(
     fake_uuid,
     mocker,
 ):
-    service_one['can_send_letters'] = True
+    service_one['permissions'] = ['letter']
     mocker.patch('app.service_api_client.get_service', return_value={"data": service_one})
     mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1)
     mocked_preview = mocker.patch(
@@ -1139,7 +1139,7 @@ def test_should_show_preview_letter_message(
     fake_uuid,
     mocker,
 ):
-    service_one['can_send_letters'] = True
+    service_one['permissions'] = ['letter']
     mocker.patch('app.service_api_client.get_service', return_value={"data": service_one})
     mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1)
 

tests/app/main/views/test_service_settings.py

@@ -62,7 +62,7 @@ def test_should_show_overview(
 
 
 @pytest.mark.parametrize('permissions, expected_rows', [
-    (['email', 'sms', 'inbound_sms'], [
+    (['email', 'sms', 'inbound_sms', 'international_sms'], [
         'Service name service one Change',
         'Email reply to address test@example.com Change',
         'Text message sender elevenchars',
@@ -75,7 +75,7 @@ def test_should_show_overview(
         'Service name service one Change',
         'Email reply to address test@example.com Change',
         'Text message sender elevenchars Change',
-        'International text messages On Change',
+        'International text messages Off Change',
         'Receive text messages Off Change',
         'Letters Off Change',
     ]),
@@ -92,7 +92,6 @@ def test_should_show_overview_for_service_with_more_things_set(
 ):
     client.login(active_user_with_permissions, mocker, service_with_reply_to_addresses)
     service_with_reply_to_addresses['permissions'] = permissions
-    service_with_reply_to_addresses['can_send_international_sms'] = True
     response = client.get(url_for(
         'main.service_settings', service_id=service_with_reply_to_addresses['id']
     ))
@@ -158,8 +157,10 @@ def test_if_can_receive_inbound_then_cant_change_sms_sender(
     response = logged_in_client.get(url_for(
         'main.service_settings', service_id=service_one['id']
     ))
-    assert 'Text message sender SomeNumber Change' not in response.get_data(as_text=True)
-    assert url_for('.service_set_sms_sender', 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 SomeNumber 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 'SomeNumber' in response.get_data(as_text=True)
 
@@ -170,7 +171,7 @@ def test_letter_contact_block_shows_none_if_not_set(
     mocker,
     mock_get_letter_organisations,
 ):
-    service_one['can_send_letters'] = True
+    service_one['permissions'] = ['letter']
     response = logged_in_client.get(url_for(
         'main.service_settings', service_id=service_one['id']
     ))
@@ -187,7 +188,7 @@ def test_escapes_letter_contact_block(
     mocker,
     mock_get_letter_organisations,
 ):
-    service_one['can_send_letters'] = True
+    service_one['permissions'] = ['letter']
     service_one['letter_contact_block'] = 'foo\nbar<script>alert(1);</script>'
     response = logged_in_client.get(url_for(
         'main.service_settings', service_id=service_one['id']
@@ -833,7 +834,7 @@ def test_cant_set_letter_contact_block_if_service_cant_send_letters(
     service_one,
     method
 ):
-    assert not service_one['can_send_letters']
+    assert 'letter' not in service_one['permissions']
     response = getattr(logged_in_client, method)(
         url_for('main.service_set_letter_contact_block', service_id=service_one['id'])
     )
@@ -844,7 +845,7 @@ def test_set_letter_contact_block_prepopulates(
     logged_in_client,
     service_one
 ):
-    service_one['can_send_letters'] = True
+    service_one['permissions'] = ['letter']
     service_one['letter_contact_block'] = 'foo bar baz waz'
     response = logged_in_client.get(url_for('main.service_set_letter_contact_block', service_id=service_one['id']))
     assert response.status_code == 200
@@ -856,7 +857,7 @@ def test_set_letter_contact_block_saves(
     service_one,
     mock_update_service,
 ):
-    service_one['can_send_letters'] = True
+    service_one['permissions'] = ['letter']
     response = logged_in_client.post(
         url_for('main.service_set_letter_contact_block', service_id=service_one['id']),
         data={'letter_contact_block': 'foo bar baz waz'}
@@ -871,7 +872,7 @@ def test_set_letter_contact_block_redirects_to_template(
     service_one,
     mock_update_service,
 ):
-    service_one['can_send_letters'] = True
+    service_one['permissions'] = ['letter']
     fake_template_id = uuid.uuid4()
     response = logged_in_client.post(
         url_for(
@@ -895,7 +896,7 @@ def test_set_letter_contact_block_has_max_10_lines(
     service_one,
     mock_update_service,
 ):
-    service_one['can_send_letters'] = True
+    service_one['permissions'] = ['letter']
     response = logged_in_client.post(
         url_for('main.service_set_letter_contact_block', service_id=service_one['id']),
         data={'letter_contact_block': '\n'.join(map(str, range(0, 11)))}
@@ -1034,7 +1035,8 @@ def test_switch_service_enable_letters(
 
     assert response.status_code == 302
     assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True)
-    assert mocked_fn.call_args == call(service_one['id'], {'can_send_letters': True})
+    assert 'letter' in mocked_fn.call_args[0][1]['permissions']
+    assert mocked_fn.call_args[0][0] == service_one['id']
 
 
 def test_switch_service_disable_letters(
@@ -1042,7 +1044,7 @@ def test_switch_service_disable_letters(
     service_one,
     mocker,
 ):
-    service_one['can_send_letters'] = True
+    service_one['permissions'] = ['letter']
     mocked_fn = mocker.patch('app.service_api_client.update_service_with_properties', return_value=service_one)
 
     response = logged_in_platform_admin_client.get(
@@ -1051,7 +1053,7 @@ def test_switch_service_disable_letters(
 
     assert response.status_code == 302
     assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True)
-    assert mocked_fn.call_args == call(service_one['id'], {"can_send_letters": False})
+    assert mocked_fn.call_args == call(service_one['id'], {"permissions": []})
 
 
 def test_switch_service_enable_international_sms(
@@ -1067,7 +1069,8 @@ def test_switch_service_enable_international_sms(
 
     assert response.status_code == 302
     assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True)
-    assert mocked_fn.call_args == call(service_one['id'], {'can_send_international_sms': True})
+    assert 'international_sms' in mocked_fn.call_args[0][1]['permissions']
+    assert mocked_fn.call_args[0][0] == service_one['id']
 
 
 def test_switch_service_disable_international_sms(
@@ -1075,7 +1078,7 @@ def test_switch_service_disable_international_sms(
     service_one,
     mocker,
 ):
-    service_one['can_send_international_sms'] = True
+    service_one['permissions'] = ['international_sms']
     mocked_fn = mocker.patch('app.service_api_client.update_service_with_properties', return_value=service_one)
 
     response = logged_in_platform_admin_client.get(
@@ -1084,7 +1087,7 @@ def test_switch_service_disable_international_sms(
 
     assert response.status_code == 302
     assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True)
-    assert mocked_fn.call_args == call(service_one['id'], {"can_send_international_sms": False})
+    assert mocked_fn.call_args == call(service_one['id'], {"permissions": []})
 
 
 def test_set_new_inbound_api_and_valid_bearer_token_calls_create_inbound_api_endpoint(

tests/conftest.py

@@ -83,7 +83,7 @@ def mock_get_service(mocker, api_user_active):
 @pytest.fixture(scope='function')
 def mock_get_international_service(mocker, api_user_active):
     def _get(service_id):
-        service = service_json(service_id, users=[api_user_active.id], can_send_international_sms=True)
+        service = service_json(service_id, users=[api_user_active.id], permissions=['international_sms'])
         return {'data': service}
 
     return mocker.patch('app.service_api_client.get_service', side_effect=_get)