Merge branch 'master' into error-handling

Conflicts:
	app/main/views/invites.py

app/assets/javascripts/highlightTags.js

@@ -33,7 +33,7 @@
     };
 
     this.update = () => this.$backgroundMaskForeground.html(
-      this.$textbox.val().replace(
+      $('<div/>').text(this.$textbox.val()).html().replace(
         tagPattern, match => `<span class='tag'>${match}</span>`
       )
     );

app/main/dao/templates_dao.py

@@ -1,7 +1,6 @@
-from flask import url_for, abort
+from flask import url_for
 from app import notifications_api_client
 from app.utils import BrowsableItem
-from notifications_python_client.errors import HTTPError
 
 
 def insert_service_template(name, type_, content, service_id, subject=None):

app/main/forms.py

@@ -98,13 +98,26 @@ class RegisterUserFromInviteForm(Form):
     email_address = HiddenField('email_address')
 
 
+# WTF forms does not give a handy way to customise error messages for
+# radio button fields so just overriding the default here for use
+# in permissions form.
+class CustomRadioField(RadioField):
+
+    def pre_validate(self, form):
+        for v, _ in self.choices:
+            if self.data == v:
+                break
+        else:
+            raise ValueError(self.gettext('Choose yes or no'))
+
+
 class PermisisonsForm(Form):
 
     # TODO fix this Radio field so we are not having to test for yes or no rather
     # use operator equality.
-    send_messages = RadioField("Send messages", choices=[('yes', 'yes'), ('no', 'no')])
+    send_messages = CustomRadioField("Send messages", choices=[('yes', 'yes'), ('no', 'no')])
-    manage_service = RadioField("Manage service", choices=[('yes', 'yes'), ('no', 'no')])
+    manage_service = CustomRadioField("Manage service", choices=[('yes', 'yes'), ('no', 'no')])
-    manage_api_keys = RadioField("Manage API keys", choices=[('yes', 'yes'), ('no', 'no')])
+    manage_api_keys = CustomRadioField("Manage API keys", choices=[('yes', 'yes'), ('no', 'no')])
 
 
 class InviteUserForm(PermisisonsForm):
@@ -192,7 +205,24 @@ class AddServiceForm(Form):
 
 
 class ServiceNameForm(Form):
-    name = StringField(u'New name')
+    def __init__(self, names_func, *args, **kwargs):
+        """
+        Keyword arguments:
+        names_func -- Returns a list of unique service_names already registered
+        on the system.
+        """
+        self._names_func = names_func
+        super(ServiceNameForm, self).__init__(*args, **kwargs)
+
+    name = StringField(
+        u'New name',
+        validators=[
+            DataRequired(message='Service name can’t be empty')
+        ])
+
+    def validate_name(self, a):
+        if a.data in self._names_func():
+            raise ValidationError('This service name is already in use')
 
 
 class ConfirmPasswordForm(Form):

app/main/views/invites.py

@@ -2,8 +2,8 @@ from flask import (
     redirect,
     url_for,
     session,
-    render_template
+    flash,
-)
+    render_template)
 
 
 from app.main import main
@@ -17,6 +17,7 @@ from app import (
 @main.route("/invitation/<token>")
 def accept_invite(token):
     invited_user = invite_api_client.check_token(token)
+
     if invited_user.status == 'cancelled':
         from_user = user_api_client.get_user(invited_user.from_user)
         service = get_service_by_id_or_404(invited_user.service)
@@ -24,6 +25,11 @@ def accept_invite(token):
                                from_user=from_user.name,
                                service_name=service['name'])
 
+    if invited_user.status == 'accepted':
+        session.pop('invited_user', None)
+        flash('You have already accepted this invitation', 'default')
+        return redirect(url_for('main.service_dashboard', service_id=invited_user.service))
+
     existing_user = user_api_client.get_user_by_email(invited_user.email_address)
     session['invited_user'] = invited_user.serialize()
 

app/main/views/manage_users.py

@@ -100,28 +100,6 @@ def edit_user_permissions(service_id, user_id):
     )
 
 
-@main.route("/services/<service_id>/users/<user_id>/delete", methods=['GET', 'POST'])
-@login_required
-@user_has_permissions('manage_users', 'manage_templates', 'manage_settings')
-def delete_user(service_id, user_id):
-    user = user_api_client.get_user(user_id)
-    service = get_service_by_id(service_id)
-
-    if request.method == 'POST':
-        return redirect(url_for('.manage_users', service_id=service_id))
-
-    flash(
-        'Are you sure you want to delete {}’s account?'.format(user.get('name') or user['email_localpart']),
-        'delete'
-    )
-
-    return render_template(
-        'views/invite-user.html',
-        user=user,
-        service_id=service_id
-    )
-
-
 @main.route("/services/<service_id>/cancel-invited-user/<invited_user_id>", methods=['GET'])
 @user_has_permissions('manage_users', 'manage_templates', 'manage_settings')
 def cancel_invited_user(service_id, invited_user_id):

app/main/views/register.py

@@ -28,9 +28,14 @@ def register():
 
     form = RegisterUserForm()
     if form.validate_on_submit():
-        return _do_registration(form)
+        registered = _do_registration(form)
-    else:
+        if registered:
-        return render_template('views/register.html', form=form)
+            return redirect(url_for('main.verify'))
+        else:
+            flash('There was an error registering your account')
+            return render_template('views/register.html', form=form), 400
+
+    return render_template('views/register.html', form=form)
 
 
 @main.route('/register-from-invite', methods=['GET', 'POST'])
@@ -44,7 +49,11 @@ def register_from_invite():
     if form.validate_on_submit():
         if form.service.data != invited_user['service'] or form.email_address.data != invited_user['email_address']:
             abort(400)
-        return _do_registration(form)
+        registered = _do_registration(form)
+        if registered:
+            return redirect(url_for('main.verify'))
+        else:
+            flash('There was an error registering your account')
 
     form.service.data = invited_user['service']
     form.email_address.data = invited_user['email_address']
@@ -68,6 +77,6 @@ def _do_registration(form, service=None):
         users_dao.send_verify_code(user.id, 'email', user.email_address)
         session['expiry_date'] = str(datetime.now() + timedelta(hours=1))
         session['user_details'] = {"email": user.email_address, "id": user.id}
-        return redirect(url_for('main.verify'))
+        return True
     else:
-        flash('There was an error registering your account')
+        return False

app/main/views/service_settings.py

@@ -12,11 +12,13 @@ from flask_login import (
     login_required,
     current_user
 )
+from notifications_python_client import HTTPError
 
 from app.main.dao.services_dao import (
     get_service_by_id,
     delete_service,
-    update_service
+    update_service,
+    find_all_service_names
 )
 
 from app.main import main
@@ -44,7 +46,7 @@ def service_settings(service_id):
 def service_name_change(service_id):
     service = get_service_by_id(service_id)['data']
 
-    form = ServiceNameForm()
+    form = ServiceNameForm(find_all_service_names)
 
     if form.validate_on_submit():
         session['service_name_change'] = form.name.data
@@ -70,10 +72,20 @@ def service_name_change_confirm(service_id):
 
     if form.validate_on_submit():
         service['name'] = session['service_name_change']
-        update_service(service)
+        try:
-        session['service_name'] = service['name']
+            update_service(service)
-        session.pop('service_name_change')
+        except HTTPError as e:
-        return redirect(url_for('.service_settings', service_id=service_id))
+            error_msg = "Duplicate service name '{}'".format(session['service_name_change'])
+            if e.status_code == 400 and error_msg in e.message['name']:
+                # Redirect the user back to the change service name screen
+                flash('This service name is already in use', 'error')
+                return redirect(url_for('main.service_name_change', service_id=service_id))
+            else:
+                raise e
+        else:
+            session['service_name'] = service['name']
+            session.pop('service_name_change')
+            return redirect(url_for('.service_settings', service_id=service_id))
     return render_template(
         'views/service-settings/confirm.html',
         heading='Change your service name',

app/main/views/styleguide.py

@@ -1,6 +1,7 @@
 from flask import render_template, current_app, abort
 from flask_wtf import Form
 from wtforms import StringField, PasswordField, TextAreaField, FileField, validators
+from app.main.forms import CustomRadioField
 from utils.template import Template
 from app.main import main
 
@@ -17,11 +18,14 @@ def styleguide():
         code = StringField('Enter code')
         message = TextAreaField(u'Message')
         file_upload = FileField('Upload a CSV file to add your recipients’ details')
+        manage_service = CustomRadioField('Manage service', choices=[('yes', 'yes'), ('no', 'no')])
+        manage_templates = CustomRadioField('Manage templates', choices=[('yes', 'yes'), ('no', 'no')])
 
     sms = "Your vehicle tax for ((registration number)) is due on ((date)). Renew online at www.gov.uk/vehicle-tax"
 
     form = FormExamples()
     form.message.data = sms
+    form.manage_service.data = 'yes'
     form.validate()
 
     template = Template({'content': sms})

app/templates/components/yes-no.html

@@ -1,15 +1,20 @@
-{% macro yes_no(name, label, current_value=None) %}
+{% macro yes_no(field, current_value=None) %}
-  <fieldset class='yes-no'>
+  <fieldset class='yes-no {% if field.errors %} error{% endif %}'>
     <legend class='yes-no-label'>
-      {{ label }}
+      {{ field.label }}
+       {% if field.errors %}
+        <span class="error-message">
+          {{ field.errors[0] }}
+        </span>
+      {% endif %}
     </legend>
     <div class='yes-no-fields inline'>
       <label class='block-label'>
-        <input type='radio' name='{{ name }}' value='yes' {% if current_value == 'yes' %}checked{% endif %} />
+        <input type='radio' name='{{ field.name }}' value='yes' {% if current_value == 'yes' %}checked{% endif %} />
         Yes
       </label>
       <label class='block-label'>
-        <input type='radio' name='{{ name }}' value='no' {% if current_value == 'no' %}checked{% endif %} />
+        <input type='radio' name='{{ field.name }}' value='no' {% if current_value == 'no' %}checked{% endif %} />
         No
       </label>
     </div>

app/templates/views/edit-user-permissions.html

@@ -21,22 +21,16 @@ Manage users – GOV.UK Notify
           Permissions
         </legend>
         <span class="form-hint">All team members can see message history</span>
-        {{ yes_no(form.send_messages.name, form.send_messages.label, form.send_messages.data) }}
+        {{ yes_no(form.send_messages, form.send_messages.data) }}
-        {{ yes_no(form.manage_service.name, form.manage_service.label, form.manage_service.data) }}
+        {{ yes_no(form.manage_service, form.manage_service.data) }}
-        {{ yes_no(form.manage_api_keys.name, form.manage_api_keys.label, form.manage_api_keys.data) }}
+        {{ yes_no(form.manage_api_keys, form.manage_api_keys.data) }}
       </fieldset>
 
-      {% if user %}
+      {{ page_footer(
-        {{ page_footer(
+        'Save',
-          'Save',
+        back_link=url_for('.manage_users', service_id=service_id),
-          delete_link=url_for('.delete_user', service_id=service_id, user_id=user_id),
+        back_link_text='Cancel'
-          delete_link_text='Delete this account',
+      ) }}
-          back_link=url_for('.manage_users', service_id=service_id),
-          back_link_text='Cancel'
-        ) }}
-      {% else %}
-        {{ page_footer('Send invitation email') }}
-      {% endif %}
 
     </form>
   </div>

app/templates/views/invite-user.html

@@ -23,22 +23,12 @@ Manage users – GOV.UK Notify
           Permissions
         </legend>
         <span class="form-hint">All team members can see message history</span>
-        {{ yes_no(form.send_messages.name, form.send_messages.label, form.send_messages.data) }}
+        {{ yes_no(form.send_messages, form.send_messages.data) }}
-        {{ yes_no(form.manage_service.name, form.manage_service.label, form.manage_service.data) }}
+        {{ yes_no(form.manage_service, form.manage_service.data) }}
-        {{ yes_no(form.manage_api_keys.name, form.manage_api_keys.label, form.manage_api_keys.data) }}
+        {{ yes_no(form.manage_api_keys, form.manage_api_keys.data) }}
       </fieldset>
 
-      {% if user %}
+      {{ page_footer('Send invitation email') }}
-        {{ page_footer(
-          'Save',
-          delete_link=url_for('.delete_user', service_id=service_id, user_id=user_id),
-          delete_link_text='Delete this account',
-          back_link=url_for('.manage_users', service_id=service_id),
-          back_link_text='Cancel'
-        ) }}
-      {% else %}
-        {{ page_footer('Send invitation email') }}
-      {% endif %}
 
     </form>
   </div>

app/templates/views/styleguide.html

@@ -247,8 +247,8 @@
   <div class="grid-row">
     <div class='column-half'>
       <fieldset class='yes-no-wrapper'>
-        {{ yes_no('manage_service', 'Manage service', True) }}
+        {{ yes_no(form.manage_service, form.manage_service.data) }}
-        {{ yes_no('templates', 'Create templates', True) }}
+        {{ yes_no(form.manage_templates, form.manage_templates.data) }}
       </fieldset>
     </div>
   </div>

tests/app/main/views/test_accept_invite.py

@@ -3,6 +3,8 @@ from flask import url_for
 from bs4 import BeautifulSoup
 
 import app
+
+from app.notify_client.models import InvitedUser
 from tests.conftest import sample_invite as create_sample_invite
 from tests.conftest import mock_check_invite_token as mock_check_token_invite
 
@@ -34,6 +36,25 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard(app_,
             assert response.location == expected_redirect_location
 
 
+def test_existing_user_cant_accept_twice(app_,
+                                         mocker,
+                                         sample_invite):
+
+    sample_invite['status'] = 'accepted'
+    invite = InvitedUser(**sample_invite)
+    mocker.patch('app.invite_api_client.check_token', return_value=invite)
+
+    with app_.test_request_context():
+        with app_.test_client() as client:
+            response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken'), follow_redirects=True)
+            assert response.status_code == 200
+            page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+            assert page.h1.string.strip() == 'Sign in'
+            flash_banners = page.find_all('div', class_='banner-default')
+            assert len(flash_banners) == 2
+            assert flash_banners[0].text.strip() == 'You have already accepted this invitation'
+
+
 def test_existing_signed_out_user_accept_invite_redirects_to_sign_in(app_,
                                                                      service_one,
                                                                      api_user_active,

tests/app/main/views/test_register.py

@@ -1,4 +1,5 @@
 from flask import url_for
+from bs4 import BeautifulSoup
 
 
 def test_render_register_returns_template_with_form(app_):
@@ -106,3 +107,24 @@ def test_should_return_400_if_password_is_blacklisted(app_,
 
     response.status_code == 200
     assert 'That password is blacklisted, too common' in response.get_data(as_text=True)
+
+
+def test_register_with_existing_email_returns_error(app_,
+                                                    api_user_active,
+                                                    mock_get_user_by_email):
+    user_data = {
+        'name': 'Already Hasaccount',
+        'email_address': api_user_active.email_address,
+        'mobile_number': '+4407700900460',
+        'password': 'validPassword!'
+    }
+
+    with app_.test_request_context():
+        response = app_.test_client().post(url_for('main.register'),
+                                           data=user_data)
+        assert response.status_code == 400
+        page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+        element = page.find('h1')
+        assert element.text == 'Create an account'
+        flash_banner = page.find('div', class_='banner-dangerous').string.strip()
+        assert flash_banner == 'There was an error registering your account'

tests/app/main/views/test_service_settings.py

@@ -54,8 +54,9 @@ def test_should_redirect_after_change_service_name(app_,
         with app_.test_client() as client:
             client.login(api_user_active)
             service_id = 123
-            response = client.post(url_for(
+            response = client.post(
-                'main.service_name_change', service_id=service_id))
+                url_for('main.service_name_change', service_id=service_id),
+                data={'name': "new name"})
 
         assert response.status_code == 302
         settings_url = url_for(
@@ -64,6 +65,27 @@ def test_should_redirect_after_change_service_name(app_,
         assert mock_get_service.called
 
 
+def test_should_not_allow_duplicate_names(app_,
+                                          api_user_active,
+                                          mock_get_service,
+                                          mock_get_user,
+                                          mock_get_user_by_email,
+                                          mock_login,
+                                          mock_has_permissions,
+                                          mock_get_services):
+    with app_.test_request_context():
+        with app_.test_client() as client:
+            client.login(api_user_active)
+            service_id = 123
+            response = client.post(
+                url_for('main.service_name_change', service_id=service_id),
+                data={'name': "service_one"})
+
+        assert response.status_code == 200
+        resp_data = response.get_data(as_text=True)
+        assert 'This service name is already in use' in resp_data
+
+
 def test_should_show_service_name_confirmation(app_,
                                                api_user_active,
                                                mock_get_service,
@@ -112,6 +134,32 @@ def test_should_redirect_after_service_name_confirmation(app_,
         assert mock_update_service.called
 
 
+def test_should_raise_duplicate_name_handled(app_,
+                                             api_user_active,
+                                             mock_get_service,
+                                             mock_update_service_raise_httperror_duplicate_name,
+                                             mock_get_user,
+                                             mock_get_user_by_email,
+                                             mock_login,
+                                             mock_verify_password,
+                                             mock_has_permissions):
+    with app_.test_request_context():
+        with app_.test_client() as client:
+            client.login(api_user_active)
+            service_id = 123
+            service_new_name = 'New Name'
+            with client.session_transaction() as session:
+                session['service_name_change'] = service_new_name
+            response = client.post(url_for(
+                'main.service_name_change_confirm', service_id=service_id))
+
+        assert response.status_code == 302
+        name_change_url = url_for(
+            'main.service_name_change', service_id=service_id, _external=True)
+        resp_data = response.get_data(as_text=True)
+        assert name_change_url == response.location
+
+
 def test_should_show_request_to_go_live(app_,
                                         api_user_active,
                                         mock_get_service,

tests/conftest.py

@@ -1,5 +1,6 @@
 import uuid
 from datetime import date, datetime, timedelta
+from unittest.mock import Mock
 import pytest
 
 from app import create_app
@@ -18,6 +19,8 @@ from app.notify_client.models import (
     InvitedUser
 )
 
+from notifications_python_client.errors import HTTPError
+
 
 @pytest.fixture(scope='session')
 def app_(request):
@@ -90,6 +93,24 @@ def mock_update_service(mocker):
         'app.notifications_api_client.update_service', side_effect=_update)
 
 
+@pytest.fixture(scope='function')
+def mock_update_service_raise_httperror_duplicate_name(mocker):
+
+    def _update(service_id,
+                service_name,
+                active,
+                limit,
+                restricted,
+                users):
+        json_mock = Mock(return_value={'message': {'name': ["Duplicate service name '{}'".format(service_name)]}})
+        resp_mock = Mock(status_code=400, json=json_mock)
+        http_error = HTTPError(response=resp_mock, message="Default message")
+        raise http_error
+
+    return mocker.patch(
+        'app.notifications_api_client.update_service', side_effect=_update)
+
+
 SERVICE_ONE_ID = "596364a0-858e-42c8-9062-a8fe822260eb"
 SERVICE_TWO_ID = "147ad62a-2951-4fa1-9ca0-093cd1a52c52"