diff --git a/app/assets/javascripts/highlightTags.js b/app/assets/javascripts/highlightTags.js index f0f46fcd9..a1ecf2f37 100644 --- a/app/assets/javascripts/highlightTags.js +++ b/app/assets/javascripts/highlightTags.js @@ -33,7 +33,7 @@ }; this.update = () => this.$backgroundMaskForeground.html( - this.$textbox.val().replace( + $('
').text(this.$textbox.val()).html().replace( tagPattern, match => `${match}` ) ); diff --git a/app/main/dao/templates_dao.py b/app/main/dao/templates_dao.py index d75a6c998..c6a22ccf2 100644 --- a/app/main/dao/templates_dao.py +++ b/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): diff --git a/app/main/forms.py b/app/main/forms.py index 7548c680c..1dbf8594f 100644 --- a/app/main/forms.py +++ b/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')]) - manage_service = RadioField("Manage service", choices=[('yes', 'yes'), ('no', 'no')]) - manage_api_keys = RadioField("Manage API keys", choices=[('yes', 'yes'), ('no', 'no')]) + send_messages = CustomRadioField("Send messages", choices=[('yes', 'yes'), ('no', 'no')]) + manage_service = CustomRadioField("Manage service", 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): diff --git a/app/main/views/invites.py b/app/main/views/invites.py index 3f079e96e..0bc7ad3c5 100644 --- a/app/main/views/invites.py +++ b/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/") 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() diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index fbb5377cc..f40b81637 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -100,28 +100,6 @@ def edit_user_permissions(service_id, user_id): ) -@main.route("/services//users//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//cancel-invited-user/", methods=['GET']) @user_has_permissions('manage_users', 'manage_templates', 'manage_settings') def cancel_invited_user(service_id, invited_user_id): diff --git a/app/main/views/register.py b/app/main/views/register.py index 924fa1d55..f9d0fa47c 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -28,9 +28,14 @@ def register(): form = RegisterUserForm() if form.validate_on_submit(): - return _do_registration(form) - else: - return render_template('views/register.html', form=form) + registered = _do_registration(form) + if registered: + 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 diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index f11fe7bf9..6c7232e77 100644 --- a/app/main/views/service_settings.py +++ b/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) - session['service_name'] = service['name'] - session.pop('service_name_change') - return redirect(url_for('.service_settings', service_id=service_id)) + try: + update_service(service) + except HTTPError as e: + 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', diff --git a/app/main/views/styleguide.py b/app/main/views/styleguide.py index eed30b20e..7d6186aa1 100644 --- a/app/main/views/styleguide.py +++ b/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}) diff --git a/app/templates/components/yes-no.html b/app/templates/components/yes-no.html index 0470ab3d6..0d58d14a0 100644 --- a/app/templates/components/yes-no.html +++ b/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) %} +
- {{ label }} + {{ field.label }} + {% if field.errors %} + + {{ field.errors[0] }} + + {% endif %}
diff --git a/app/templates/views/edit-user-permissions.html b/app/templates/views/edit-user-permissions.html index 291d7fa62..af8c94367 100644 --- a/app/templates/views/edit-user-permissions.html +++ b/app/templates/views/edit-user-permissions.html @@ -21,22 +21,16 @@ Manage users – GOV.UK Notify Permissions All team members can see message history - {{ yes_no(form.send_messages.name, form.send_messages.label, form.send_messages.data) }} - {{ yes_no(form.manage_service.name, form.manage_service.label, form.manage_service.data) }} - {{ yes_no(form.manage_api_keys.name, form.manage_api_keys.label, form.manage_api_keys.data) }} + {{ yes_no(form.send_messages, form.send_messages.data) }} + {{ yes_no(form.manage_service, form.manage_service.data) }} + {{ yes_no(form.manage_api_keys, form.manage_api_keys.data) }}
- {% if user %} - {{ 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 %} + {{ page_footer( + 'Save', + back_link=url_for('.manage_users', service_id=service_id), + back_link_text='Cancel' + ) }}
diff --git a/app/templates/views/invite-user.html b/app/templates/views/invite-user.html index 407c2693f..d4fcf104f 100644 --- a/app/templates/views/invite-user.html +++ b/app/templates/views/invite-user.html @@ -23,22 +23,12 @@ Manage users – GOV.UK Notify Permissions All team members can see message history - {{ yes_no(form.send_messages.name, form.send_messages.label, form.send_messages.data) }} - {{ yes_no(form.manage_service.name, form.manage_service.label, form.manage_service.data) }} - {{ yes_no(form.manage_api_keys.name, form.manage_api_keys.label, form.manage_api_keys.data) }} + {{ yes_no(form.send_messages, form.send_messages.data) }} + {{ yes_no(form.manage_service, form.manage_service.data) }} + {{ yes_no(form.manage_api_keys, form.manage_api_keys.data) }} - {% if user %} - {{ 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 %} + {{ page_footer('Send invitation email') }} diff --git a/app/templates/views/styleguide.html b/app/templates/views/styleguide.html index a45f1a30e..d5addc1f9 100644 --- a/app/templates/views/styleguide.html +++ b/app/templates/views/styleguide.html @@ -247,8 +247,8 @@
- {{ yes_no('manage_service', 'Manage service', True) }} - {{ yes_no('templates', 'Create templates', True) }} + {{ yes_no(form.manage_service, form.manage_service.data) }} + {{ yes_no(form.manage_templates, form.manage_templates.data) }}
diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 3e48e1f73..eaa796198 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/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, diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 5a9133d81..9849da99f 100644 --- a/tests/app/main/views/test_register.py +++ b/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' diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index f0210dd7e..65f7030e7 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/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( - 'main.service_name_change', service_id=service_id)) + response = client.post( + 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, diff --git a/tests/conftest.py b/tests/conftest.py index 93dce2265..be599df6b 100644 --- a/tests/conftest.py +++ b/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"