diff --git a/app/__init__.py b/app/__init__.py index 8db5ca6d1..d0ce1d245 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -6,10 +6,12 @@ from flask import (Flask, session, Markup, escape, render_template, make_respons from flask._compat import string_types from flask_login import LoginManager from flask_wtf import CsrfProtect -from werkzeug.exceptions import abort +from notifications_python_client import HTTPError from pygments import highlight from pygments.lexers import JavascriptLexer from pygments.formatters import HtmlFormatter +from werkzeug.exceptions import abort + from app.notify_client.api_client import NotificationsAdminAPIClient from app.notify_client.api_key_api_client import ApiKeyApiClient from app.notify_client.user_api_client import UserApiClient @@ -176,10 +178,29 @@ def useful_headers_after_request(response): def register_errorhandlers(application): - def render_error(error): - # If a HTTPException, pull the `code` attribute; default to 500 - error_code = getattr(error, 'code', 500) + def _error_response(error_code): resp = make_response(render_template("error/{0}.html".format(error_code)), error_code) return useful_headers_after_request(resp) - for errcode in [401, 404, 403, 500]: - application.errorhandler(errcode)(render_error) + + @application.errorhandler(HTTPError) + def render_http_error(error): + error_code = error.status_code + if error_code not in [401, 404, 403, 500]: + error_code = 500 + return _error_response(error_code) + + @application.errorhandler(404) + def handle_not_found(error): + return _error_response(404) + + @application.errorhandler(403) + def handle_not_authorized(error): + return _error_response(403) + + @application.errorhandler(401) + def handle_no_permissions(error): + return _error_response(401) + + @application.errorhandler(Exception) + def handle_bad_request(error): + return _error_response(500) 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/assets/stylesheets/_grids.scss b/app/assets/stylesheets/_grids.scss index 5e8643173..490f4d01d 100644 --- a/app/assets/stylesheets/_grids.scss +++ b/app/assets/stylesheets/_grids.scss @@ -2,7 +2,6 @@ @include grid-column(3/4); } - .column-one-eighth { @include grid-column(1/8); } @@ -19,3 +18,11 @@ .bottom-gutter-2-3 { margin-bottom: $gutter * 2/3; } + +.align-with-heading { + display: block; + text-align: center; + margin-top: 45px; + padding-left: 2px; + padding-right: 2px; +} diff --git a/app/assets/stylesheets/components/yes-no.scss b/app/assets/stylesheets/components/yes-no.scss index 9a71113a5..9dd2101b7 100644 --- a/app/assets/stylesheets/components/yes-no.scss +++ b/app/assets/stylesheets/components/yes-no.scss @@ -9,8 +9,14 @@ padding: 10px 0; &-label { + padding-top: 19px; float: left; + + &.error { + padding-top: 5px; + } + } &-fields { diff --git a/app/assets/stylesheets/views/edit-template.scss b/app/assets/stylesheets/views/edit-template.scss index 3635b067f..dd1c8e789 100644 --- a/app/assets/stylesheets/views/edit-template.scss +++ b/app/assets/stylesheets/views/edit-template.scss @@ -3,7 +3,7 @@ &-placeholder-hint { display: block; padding-top: 20px; - color: $secondary-text-colour; + //color: $secondary-text-colour; } } diff --git a/app/main/dao/services_dao.py b/app/main/dao/services_dao.py index 4104b0ff4..39417fceb 100644 --- a/app/main/dao/services_dao.py +++ b/app/main/dao/services_dao.py @@ -1,14 +1,6 @@ -from flask import url_for, abort, current_app +from flask import url_for, current_app from app import notifications_api_client from app.utils import BrowsableItem -from notifications_python_client.errors import HTTPError - - -def insert_new_service(service_name, user_id): - resp = notifications_api_client.create_service( - service_name, False, current_app.config['DEFAULT_SERVICE_LIMIT'], True, user_id) - - return resp['data']['id'] def update_service(service): @@ -26,15 +18,7 @@ def get_service_by_id(id_): def get_service_by_id_or_404(id_): - try: - return notifications_api_client.get_service(id_)['data'] - except KeyError: - abort(404) - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + return notifications_api_client.get_service(id_)['data'] def get_services(user_id=None): diff --git a/app/main/dao/templates_dao.py b/app/main/dao/templates_dao.py index 7d07b7d0e..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): @@ -19,13 +18,7 @@ def get_service_templates(service_id): def get_service_template_or_404(service_id, template_id): - try: - return notifications_api_client.get_service_template(service_id, template_id) - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + return notifications_api_client.get_service_template(service_id, template_id) def delete_service_template(service_id, template_id): diff --git a/app/main/dao/users_dao.py b/app/main/dao/users_dao.py index 932a98cd3..9b27d5839 100644 --- a/app/main/dao/users_dao.py +++ b/app/main/dao/users_dao.py @@ -1,11 +1,6 @@ -from datetime import datetime from notifications_python_client import HTTPError -from sqlalchemy.orm import load_only - from app import login_manager -from app.main.encryption import hashpw - from app import user_api_client # diff --git a/app/main/forms.py b/app/main/forms.py index 474f90209..640e993de 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -150,6 +150,23 @@ class TwoFactorForm(Form): raise ValidationError(reason) +class VerifySmsForm(Form): + def __init__(self, validate_code_func, *args, **kwargs): + ''' + Keyword arguments: + validate_code_func -- Validates the code with the API. + ''' + self.validate_code_func = validate_code_func + super(VerifySmsForm, self).__init__(*args, **kwargs) + + sms_code = sms_code() + + def validate_sms_code(self, field): + is_valid, reason = self.validate_code_func(field.data, 'sms') + if not is_valid: + raise ValidationError(reason) + + class VerifyForm(Form): def __init__(self, validate_code_func, *args, **kwargs): ''' @@ -168,10 +185,12 @@ class VerifyForm(Form): raise ValidationError(reason) def validate_email_code(self, field): - self._validate_code(field.data, 'email') + if self.sms_code.data: + self._validate_code(field.data, 'email') def validate_sms_code(self, field): - self._validate_code(field.data, 'sms') + if self.email_code.data: + self._validate_code(field.data, 'sms') class EmailNotReceivedForm(Form): @@ -205,7 +224,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/add_service.py b/app/main/views/add_service.py index 937c28310..415ef4c30 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -2,8 +2,8 @@ from flask import ( render_template, redirect, session, - url_for -) + url_for, + current_app) from flask_login import login_required @@ -14,8 +14,8 @@ from app.notify_client.models import InvitedUser from app import ( invite_api_client, - user_api_client -) + user_api_client, + notifications_api_client) @main.route("/add-service", methods=['GET', 'POST']) @@ -36,8 +36,9 @@ def add_service(): heading = 'Which service do you want to set up notifications for?' if form.validate_on_submit(): session['service_name'] = form.name.data - user = users_dao.get_user_by_id(session['user_id']) - service_id = services_dao.insert_new_service(session['service_name'], user.id) + service_id = notifications_api_client.create_service( + session['service_name'], False, current_app.config['DEFAULT_SERVICE_LIMIT'], True, session['user_id']) + return redirect(url_for('main.service_dashboard', service_id=service_id)) else: return render_template( diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 969017366..f681faa45 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -1,5 +1,4 @@ from flask import ( - abort, render_template, session, flash @@ -9,36 +8,25 @@ from flask_login import login_required from app.main import main from app.main.dao.services_dao import get_service_by_id from app.main.dao import templates_dao -from notifications_python_client.errors import HTTPError from app import job_api_client @main.route("/services//dashboard") @login_required def service_dashboard(service_id): - try: - templates = templates_dao.get_service_templates(service_id)['data'] - jobs = job_api_client.get_job(service_id)['data'] - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e - try: - service = get_service_by_id(service_id) - session['service_name'] = service['data']['name'] - session['service_id'] = service['data']['id'] + templates = templates_dao.get_service_templates(service_id)['data'] + jobs = job_api_client.get_job(service_id)['data'] + + service = get_service_by_id(service_id) + session['service_name'] = service['data']['name'] + session['service_id'] = service['data']['id'] + + if session.get('invited_user'): + session.pop('invited_user', None) + service_name = service['data']['name'] + message = 'You have successfully accepted your invitation and been added to {}'.format(service_name) + flash(message, 'default_with_tick') - if session.get('invited_user'): - session.pop('invited_user', None) - service_name = service['data']['name'] - message = 'You have sucessfully accepted your invitation and been added to {}'.format(service_name) - flash(message, 'default_with_tick') - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e return render_template( 'views/service_dashboard.html', jobs=jobs[:5], diff --git a/app/main/views/forgot_password.py b/app/main/views/forgot_password.py index 16818bf06..f63d8227f 100644 --- a/app/main/views/forgot_password.py +++ b/app/main/views/forgot_password.py @@ -1,7 +1,6 @@ from flask import ( render_template, ) -from notifications_python_client.errors import HTTPError from app.main import main from app.main.forms import ForgotPasswordForm @@ -12,11 +11,8 @@ from app import user_api_client def forgot_password(): form = ForgotPasswordForm() if form.validate_on_submit(): - try: - user_api_client.send_reset_password_url(form.email_address.data) - except HTTPError as e: - if e.status_code != 404: - raise e + user_api_client.send_reset_password_url(form.email_address.data) + return render_template('views/password-reset-sent.html') return render_template('views/forgot-password.html', form=form) diff --git a/app/main/views/invites.py b/app/main/views/invites.py index 44d00296c..0bc7ad3c5 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -2,11 +2,9 @@ from flask import ( redirect, url_for, session, - abort, - render_template -) + flash, + render_template) -from notifications_python_client.errors import HTTPError from app.main import main from app.main.dao.services_dao import get_service_by_id_or_404 @@ -18,32 +16,29 @@ from app import ( @main.route("/invitation/") def accept_invite(token): + invited_user = invite_api_client.check_token(token) - try: + 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) + return render_template('views/cancelled-invitation.html', + from_user=from_user.name, + service_name=service['name']) - 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) - return render_template('views/cancelled-invitation.html', - 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() + existing_user = user_api_client.get_user_by_email(invited_user.email_address) + session['invited_user'] = invited_user.serialize() - if existing_user: + if existing_user: - user_api_client.add_user_to_service(invited_user.service, - existing_user.id, - invited_user.permissions) - invite_api_client.accept_invite(invited_user.service, invited_user.id) - return redirect(url_for('main.service_dashboard', service_id=invited_user.service)) - else: - return redirect(url_for('main.register_from_invite')) - - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + user_api_client.add_user_to_service(invited_user.service, + existing_user.id, + invited_user.permissions) + invite_api_client.accept_invite(invited_user.service, invited_user.id) + return redirect(url_for('main.service_dashboard', service_id=invited_user.service)) + else: + return redirect(url_for('main.register_from_invite')) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 24949756e..9847ef2cc 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -8,7 +8,6 @@ from flask import ( jsonify ) from flask_login import login_required -from notifications_python_client.errors import HTTPError from utils.template import Template from app import job_api_client, notification_api_client @@ -20,89 +19,71 @@ from app.main.dao import services_dao @main.route("/services//jobs") @login_required def view_jobs(service_id): - try: - jobs = job_api_client.get_job(service_id)['data'] - return render_template( - 'views/jobs/jobs.html', - jobs=jobs, - service_id=service_id - ) - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + jobs = job_api_client.get_job(service_id)['data'] + return render_template( + 'views/jobs/jobs.html', + jobs=jobs, + service_id=service_id + ) @main.route("/services//jobs/") @login_required def view_job(service_id, job_id): service = services_dao.get_service_by_id_or_404(service_id) - try: - job = job_api_client.get_job(service_id, job_id)['data'] - template = templates_dao.get_service_template_or_404(service_id, job['template'])['data'] - notifications = notification_api_client.get_notifications_for_service(service_id, job_id) - finished = job['status'] == 'finished' - return render_template( - 'views/jobs/job.html', - notifications=notifications['notifications'], - counts={ - 'queued': 0 if finished else job['notification_count'], - 'sent': job['notification_count'] if finished else 0, - 'failed': 0, - 'cost': u'£0.00' - }, - uploaded_at=job['created_at'], - finished_at=job['updated_at'] if finished else None, - uploaded_file_name=job['original_file_name'], - template=Template( - template, - prefix=service['name'] if template['template_type'] == 'sms' else '' - ), - service_id=service_id, - service=service, - job_id=job_id - ) - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + job = job_api_client.get_job(service_id, job_id)['data'] + template = templates_dao.get_service_template_or_404(service_id, job['template'])['data'] + notifications = notification_api_client.get_notifications_for_service(service_id, job_id) + finished = job['status'] == 'finished' + return render_template( + 'views/jobs/job.html', + notifications=notifications['notifications'], + counts={ + 'queued': 0 if finished else job['notification_count'], + 'sent': job['notification_count'] if finished else 0, + 'failed': 0, + 'cost': u'£0.00' + }, + uploaded_at=job['created_at'], + finished_at=job['updated_at'] if finished else None, + uploaded_file_name=job['original_file_name'], + template=Template( + template, + prefix=service['name'] if template['template_type'] == 'sms' else '' + ), + service_id=service_id, + service=service, + job_id=job_id + ) @main.route("/services//jobs/.json") @login_required def view_job_updates(service_id, job_id): service = services_dao.get_service_by_id_or_404(service_id) - try: - job = job_api_client.get_job(service_id, job_id)['data'] - notifications = notification_api_client.get_notifications_for_service(service_id, job_id) - finished = job['status'] == 'finished' - return jsonify(**{ - 'counts': render_template( - 'partials/jobs/count.html', - counts={ - 'queued': 0 if finished else job['notification_count'], - 'sent': job['notification_count'] if finished else 0, - 'failed': 0, - 'cost': u'£0.00' - } - ), - 'notifications': render_template( - 'partials/jobs/notifications.html', - notifications=notifications['notifications'] - ), - 'status': render_template( - 'partials/jobs/status.html', - uploaded_at=job['created_at'], - finished_at=job['updated_at'] if finished else None - ), - }) - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + job = job_api_client.get_job(service_id, job_id)['data'] + notifications = notification_api_client.get_notifications_for_service(service_id, job_id) + finished = job['status'] == 'finished' + return jsonify(**{ + 'counts': render_template( + 'partials/jobs/count.html', + counts={ + 'queued': 0 if finished else job['notification_count'], + 'sent': job['notification_count'] if finished else 0, + 'failed': 0, + 'cost': u'£0.00' + } + ), + 'notifications': render_template( + 'partials/jobs/notifications.html', + notifications=notifications['notifications'] + ), + 'status': render_template( + 'partials/jobs/status.html', + uploaded_at=job['created_at'], + finished_at=job['updated_at'] if finished else None + ), + }) @main.route("/services//jobs//notification/") diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 6848794a5..f40b81637 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -11,9 +11,6 @@ from flask_login import ( current_user ) -from notifications_python_client.errors import HTTPError -from app import user_api_client - from app.main import main from app.main.forms import ( InviteUserForm, diff --git a/app/main/views/register.py b/app/main/views/register.py index 476032784..f9d0fa47c 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -11,8 +11,6 @@ from flask import ( from flask.ext.login import current_user -from notifications_python_client.errors import HTTPError - from app.main import main from app.main.dao import users_dao from app.main.forms import ( @@ -65,17 +63,10 @@ def register_from_invite(): def _do_registration(form, service=None): if users_dao.is_email_unique(form.email_address.data): - try: - user = user_api_client.register_user(form.name.data, - form.email_address.data, - form.mobile_number.data, - form.password.data) - - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + user = user_api_client.register_user(form.name.data, + form.email_address.data, + form.mobile_number.data, + form.password.data) # TODO possibly there should be some exception handling # for sending sms and email codes. diff --git a/app/main/views/send.py b/app/main/views/send.py index b7578afa5..72ee60a8c 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -15,7 +15,6 @@ from flask import ( ) from flask_login import login_required, current_user -from notifications_python_client.errors import HTTPError from utils.template import Template from utils.recipients import RecipientCSV, first_column_heading @@ -38,8 +37,8 @@ send_messages_page_headings = { manage_templates_page_headings = { - 'email': 'Manage templates', - 'sms': 'Manage templates' + 'email': 'Email templates', + 'sms': 'Text message templates' } @@ -80,13 +79,8 @@ def choose_template(service_id, template_type): if template_type not in ['email', 'sms']: abort(404) - try: - jobs = job_api_client.get_job(service_id)['data'] - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + jobs = job_api_client.get_job(service_id)['data'] + return render_template( 'views/choose-template.html', templates=[ @@ -166,7 +160,7 @@ def get_example_csv(service_id, template_id): 'email': current_user.email_address, 'sms': current_user.mobile_number }[template.template_type] - ] + ["test {}".format(header) for header in template.placeholders]) + ] + _get_fake_personalisation(template.placeholders)) return output.getvalue(), 200, {'Content-Type': 'text/csv; charset=utf-8'} @@ -183,13 +177,11 @@ def send_message_to_self(service_id, template_id): ) if template.template_type == 'sms': writer.writerow( - [current_user.mobile_number] + - ["test {}".format(header) for header in template.placeholders] + [current_user.mobile_number] + _get_fake_personalisation(template.placeholders) ) if template.template_type == 'email': writer.writerow( - [current_user.email_address] + - ["test {}".format(header) for header in template.placeholders] + [current_user.email_address] + _get_fake_personalisation(template.placeholders) ) filedata = { @@ -230,7 +222,8 @@ def check_messages(service_id, upload_id): contents, template_type=template.template_type, placeholders=template.placeholders, - max_initial_rows_shown=5 + max_initial_rows_shown=15, + max_errors_shown=15 ) with suppress(StopIteration): @@ -245,8 +238,13 @@ def check_messages(service_id, upload_id): template=template, page_heading=get_page_headings(template.template_type), errors=get_errors_for_csv(recipients, template.template_type), + rows_have_errors=any(recipients.rows_with_errors), count_of_recipients=session['upload_data']['notification_count'], - count_of_displayed_recipients=len(list(recipients.rows_annotated_and_truncated)), + count_of_displayed_recipients=( + len(list(recipients.initial_annotated_rows_with_errors)) + if any(recipients.rows_with_errors) else + len(list(recipients.initial_annotated_rows)) + ), original_file_name=session['upload_data'].get('original_file_name'), send_button_text=get_send_button_text(template.template_type, session['upload_data']['notification_count']), service_id=service_id, @@ -280,3 +278,9 @@ def start_job(service_id, upload_id): return redirect( url_for('main.view_job', service_id=service_id, job_id=upload_id) ) + + +def _get_fake_personalisation(placeholders): + return [ + "{} 1".format(header) for header in placeholders + ] diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index a57696375..6c7232e77 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -12,13 +12,13 @@ from flask_login import ( login_required, current_user ) - -from notifications_python_client.errors import HTTPError +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 @@ -31,13 +31,8 @@ from app.main.forms import ConfirmPasswordForm, ServiceNameForm @login_required @user_has_permissions('manage_settings') def service_settings(service_id): - try: - service = get_service_by_id(service_id)['data'] - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + service = get_service_by_id(service_id)['data'] + return render_template( 'views/service-settings.html', service=service, @@ -49,15 +44,9 @@ def service_settings(service_id): @login_required @user_has_permissions('manage_settings') def service_name_change(service_id): - try: - service = get_service_by_id(service_id)['data'] - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + 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 @@ -74,13 +63,7 @@ def service_name_change(service_id): @login_required @user_has_permissions('manage_settings') def service_name_change_confirm(service_id): - try: - service = get_service_by_id(service_id)['data'] - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + service = get_service_by_id(service_id)['data'] # Validate password for form def _check_password(pwd): @@ -89,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', @@ -104,13 +97,7 @@ def service_name_change_confirm(service_id): @login_required @user_has_permissions('manage_settings') def service_request_to_go_live(service_id): - try: - service = get_service_by_id(service_id)['data'] - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + service = get_service_by_id(service_id)['data'] if request.method == 'GET': return render_template( 'views/service-settings/request-to-go-live.html', @@ -127,13 +114,7 @@ def service_request_to_go_live(service_id): @login_required @user_has_permissions('manage_settings') def service_status_change(service_id): - try: - service = get_service_by_id(service_id)['data'] - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + service = get_service_by_id(service_id)['data'] if request.method == 'GET': return render_template( @@ -149,13 +130,7 @@ def service_status_change(service_id): @login_required @user_has_permissions('manage_settings') def service_status_change_confirm(service_id): - try: - service = get_service_by_id(service_id)['data'] - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + service = get_service_by_id(service_id)['data'] # Validate password for form def _check_password(pwd): @@ -178,13 +153,7 @@ def service_status_change_confirm(service_id): @login_required @user_has_permissions('manage_settings') def service_delete(service_id): - try: - service = get_service_by_id(service_id)['data'] - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + service = get_service_by_id(service_id)['data'] if request.method == 'GET': return render_template( @@ -200,13 +169,7 @@ def service_delete(service_id): @login_required @user_has_permissions('manage_settings') def service_delete_confirm(service_id): - try: - service = get_service_by_id(service_id)['data'] - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + service = get_service_by_id(service_id)['data'] # Validate password for form def _check_password(pwd): @@ -214,13 +177,7 @@ def service_delete_confirm(service_id): form = ConfirmPasswordForm(_check_password) if form.validate_on_submit(): - try: - service = delete_service(service_id) - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + service = delete_service(service_id) return redirect(url_for('.choose_service')) return render_template( diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 0b7119e9d..f390ff9af 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -13,6 +13,11 @@ form_objects = { 'sms': SMSTemplateForm } +page_headings = { + 'email': 'email', + 'sms': 'text message' +} + @main.route("/services//templates/add-", methods=['GET', 'POST']) @login_required @@ -42,7 +47,8 @@ def add_service_template(service_id, template_type): 'views/edit-{}-template.html'.format(template_type), form=form, template_type=template_type, - service_id=service_id + service_id=service_id, + heading_action='Add' ) @@ -70,7 +76,8 @@ def edit_service_template(service_id, template_id): form=form, service_id=service_id, template_id=template_id, - template_type=template['template_type'] + template_type=template['template_type'], + heading_action='Edit' ) diff --git a/app/main/views/verify.py b/app/main/views/verify.py index 63cc1cb18..6bc4c3e2c 100644 --- a/app/main/views/verify.py +++ b/app/main/views/verify.py @@ -2,17 +2,17 @@ from flask import ( render_template, redirect, session, - url_for, - abort + url_for ) -from notifications_python_client.errors import HTTPError - from flask_login import login_user from app.main import main from app.main.dao import users_dao -from app.main.forms import VerifyForm +from app.main.forms import ( + VerifyForm, + VerifySmsForm +) @main.route('/verify', methods=['GET', 'POST']) @@ -24,19 +24,19 @@ def verify(): def _check_code(code, code_type): return users_dao.check_verify_code(user_id, code, code_type) - form = VerifyForm(_check_code) + + if session.get('invited_user'): + form = VerifySmsForm(_check_code) + else: + form = VerifyForm(_check_code) + if form.validate_on_submit(): try: user = users_dao.get_user_by_id(user_id) activated_user = users_dao.activate_user(user) login_user(activated_user) return redirect(url_for('main.add_service', first='first')) - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e finally: - del session['user_details'] + session.pop('user_details', None) return render_template('views/verify.html', form=form) diff --git a/app/notify_client/api_client.py b/app/notify_client/api_client.py index 719f015b5..dbe66d2d4 100644 --- a/app/notify_client/api_client.py +++ b/app/notify_client/api_client.py @@ -27,7 +27,7 @@ class NotificationsAdminAPIClient(NotificationsAPIClient): "user_id": user_id, "restricted": restricted } - return self.post("/service", data) + return self.post("/service", data)['data']['id'] def delete_service(self, service_id): """ diff --git a/app/notify_client/models.py b/app/notify_client/models.py index 9c8093ff4..3c27c09e4 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -10,7 +10,7 @@ class User(UserMixin): self._mobile_number = fields.get('mobile_number') self._password_changed_at = fields.get('password_changed_at') self._permissions = fields.get('permissions') - self._failed_login_count = 0 + self._failed_login_count = fields.get('failed_login_count') self._state = fields.get('state') self.max_failed_login_count = max_failed_login_count @@ -129,7 +129,10 @@ class InvitedUser(object): if isinstance(permissions, list): self.permissions = permissions else: - self.permissions = permissions.split(',') + if permissions: + self.permissions = permissions.split(',') + else: + self.permissions = [] self.status = status self.created_at = created_at diff --git a/app/notify_client/sender.py b/app/notify_client/sender.py deleted file mode 100644 index 3ae7951bf..000000000 --- a/app/notify_client/sender.py +++ /dev/null @@ -1,27 +0,0 @@ -from random import randint -from flask import url_for, current_app -from itsdangerous import URLSafeTimedSerializer, SignatureExpired -from app import notifications_api_client - - -def send_change_password_email(email): - link_to_change_password = url_for('.new_password', token=generate_token(email), _external=True) - notifications_api_client.send_email(email_address=email, - from_address='notify@digital.cabinet-office.gov.uk', - message=link_to_change_password, - subject='Reset password for GOV.UK Notify') - - -def generate_token(email): - ser = URLSafeTimedSerializer(current_app.config['SECRET_KEY']) - return ser.dumps(email, current_app.config.get('DANGEROUS_SALT')) - - -def check_token(token): - ser = URLSafeTimedSerializer(current_app.config['SECRET_KEY']) - try: - email = ser.loads(token, max_age=current_app.config['TOKEN_MAX_AGE_SECONDS'], - salt=current_app.config.get('DANGEROUS_SALT')) - return email - except SignatureExpired as e: - current_app.logger.info('token expired %s' % e) diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index ba15b90ba..4ffc8456c 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -1,5 +1,3 @@ -import json - from notifications_python_client.notifications import BaseAPIClient from notifications_python_client.errors import HTTPError diff --git a/app/templates/components/yes-no.html b/app/templates/components/yes-no.html index 0d58d14a0..71718aa95 100644 --- a/app/templates/components/yes-no.html +++ b/app/templates/components/yes-no.html @@ -1,6 +1,6 @@ {% macro yes_no(field, current_value=None) %} -
- +
+ {{ field.label }} {% if field.errors %} diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 3f2727e40..bafcfae2e 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -10,9 +10,9 @@ {% elif current_user.has_permissions(['manage_templates']) %} {% endif %} {% if current_user.has_permissions(['manage_users', 'manage_settings']) %} diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 13b9e86a3..018f0f491 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -72,12 +72,12 @@ {% endif %} {% call(item) list_table( - recipients.rows_annotated_and_truncated, + recipients.initial_annotated_rows_with_errors if rows_have_errors else recipients.initial_annotated_rows, caption=original_file_name, - field_headings=['Row'] + recipients.column_headers_with_placeholders_highlighted + field_headings=['1'] + recipients.column_headers_with_placeholders_highlighted ) %} {% call field() %} - {{ item.index + 1 }} + {{ item.index + 2 }} {% endcall %} {% for column in recipients.column_headers %} {% if item[column].error %} @@ -99,7 +99,7 @@ {% if count_of_displayed_recipients < count_of_recipients %} {% endif %} diff --git a/app/templates/views/choose-template.html b/app/templates/views/choose-template.html index 00b4cbc2f..e3d11ed02 100644 --- a/app/templates/views/choose-template.html +++ b/app/templates/views/choose-template.html @@ -10,55 +10,64 @@ {% block maincolumn_content %} -

{{ page_heading }}

+ {% if not templates %} -
+

{{ page_heading }}

- {% if templates %} - {% if not has_jobs %} - {% if current_user.has_permissions(['send_texts', 'send_emails', 'send_letters'], or_=True) %} - {{ banner( - """ - Send yourself a test message - """, - subhead='Next step', - type="tip" - )}} - {% endif %} - {% endif %} -
- {% for template in templates %} -
- {% if 'email' == template_type %} - {{ email_message( - template.subject, - template.formatted_as_markup, - name=template.name - ) }} - {% elif 'sms' == template_type %} - {{ sms_message(template.formatted_as_markup, name=template.name) }} - {% endif %} -
-
- -
- {% endfor %} -
+ {% if current_user.has_permissions(['manage_templates']) %} + Add a new template {% endif %} -

- {% if current_user.has_permissions(['manage_templates']) %} - Add a new template - {% endif %} -

+ {% else %} + +
+
+

{{ page_heading }}

+
+ {% if current_user.has_permissions(['manage_templates']) %} + + {% endif %} +
+ + {% if not has_jobs %} + {% if current_user.has_permissions(['send_texts', 'send_emails', 'send_letters'], or_=True) %} + {{ banner( + """ + Send yourself a test message + """, + subhead='Next step', + type="tip" + )}} + {% endif %} + {% endif %} +
+ {% for template in templates %} +
+ {% if 'email' == template_type %} + {{ email_message( + template.subject, + template.formatted_as_markup, + name=template.name + ) }} + {% elif 'sms' == template_type %} + {{ sms_message(template.formatted_as_markup, name=template.name) }} + {% endif %} +
+
+ +
+ {% endfor %} +
+ {% endif %} -
{% endblock %} diff --git a/app/templates/views/edit-email-template.html b/app/templates/views/edit-email-template.html index 3f9ec239f..0505fc97c 100644 --- a/app/templates/views/edit-email-template.html +++ b/app/templates/views/edit-email-template.html @@ -3,12 +3,14 @@ {% from "components/page-footer.html" import page_footer %} {% block page_title %} - {{ h1 }} – GOV.UK Notify + {{ heading_action }} email template – GOV.UK Notify {% endblock %} {% block maincolumn_content %} -

Edit email template

+

+ {{ heading_action }} email template +

@@ -21,17 +23,19 @@
{{ page_footer( 'Save', delete_link=url_for('.delete_service_template', service_id=service_id, template_id=template_id) if template_id or None, - delete_link_text='Delete this template', - back_link=url_for('.choose_template', template_type=template_type, service_id=service_id), - back_link_text='Cancel' + delete_link_text='Delete this template' ) }} diff --git a/app/templates/views/edit-sms-template.html b/app/templates/views/edit-sms-template.html index b8b44a927..a571c300a 100644 --- a/app/templates/views/edit-sms-template.html +++ b/app/templates/views/edit-sms-template.html @@ -3,12 +3,14 @@ {% from "components/page-footer.html" import page_footer %} {% block page_title %} - {{ h1 }} – GOV.UK Notify + {{ heading_action }} text message template – GOV.UK Notify {% endblock %} {% block maincolumn_content %} -

Edit text message template

+

+ {{ heading_action }} text message template +

@@ -20,17 +22,19 @@
{{ page_footer( 'Save', delete_link=url_for('.delete_service_template', service_id=service_id, template_id=template_id) if template_id or None, - delete_link_text='Delete this template', - back_link=url_for('.choose_template', service_id=service_id, template_type=template_type), - back_link_text='Cancel' + delete_link_text='Delete this template' ) }}
diff --git a/app/templates/views/invite-user.html b/app/templates/views/invite-user.html index d4fcf104f..b50b9d663 100644 --- a/app/templates/views/invite-user.html +++ b/app/templates/views/invite-user.html @@ -10,7 +10,7 @@ Manage users – GOV.UK Notify {% block maincolumn_content %}

- {{ user.name or user.email_localpart or "Add a new team member" }} + {{ user.name or user.email_localpart or "Invite a team member" }}

diff --git a/app/templates/views/letters.html b/app/templates/views/letters.html index 1086f1d50..5ef3e4d6f 100644 --- a/app/templates/views/letters.html +++ b/app/templates/views/letters.html @@ -7,7 +7,11 @@ {% block maincolumn_content %}

- Send letters + {% if current_user.has_permissions(['send_letters']) %} + Send letters + {% else %} + Letter templates + {% endif %}

diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index 0876f8f5b..e0f7fe1a5 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -16,11 +16,16 @@ Manage users – GOV.UK Notify {% block maincolumn_content %} -

- Manage team -

- - Invite a team member +
+
+

+ Manage team +

+
+ +
{% call(item) list_table( users, caption='Active', **table_options diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index c285f6c7d..b05e06025 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -7,7 +7,7 @@ {% block maincolumn_content %} -

Service settings

+

Manage settings

{{ browse_list([ { diff --git a/app/templates/views/service_dashboard.html b/app/templates/views/service_dashboard.html index 90b4ecc0d..f7a6dc7fa 100644 --- a/app/templates/views/service_dashboard.html +++ b/app/templates/views/service_dashboard.html @@ -29,12 +29,12 @@
    {% if current_user.has_permissions(['manage_templates']) %}
  1. - Add a template + Add a template
  2. {% endif %} {% if current_user.has_permissions(['send_texts', 'send_emails', 'send_letters']) %}
  3. - Send yourself a text message + Send yourself a text message
  4. {% endif %}
@@ -42,7 +42,7 @@ {% elif not jobs %} {% call banner_wrapper(subhead='Next step', type="tip") %} {% if current_user.has_permissions(['send_texts', 'send_emails', 'send_letters']) %} - Send yourself a text message + Send yourself a text message {% endif %} {% endcall %} {% else %} diff --git a/app/templates/views/verify.html b/app/templates/views/verify.html index dcde0a4f3..e34285720 100644 --- a/app/templates/views/verify.html +++ b/app/templates/views/verify.html @@ -3,30 +3,48 @@ {% from "components/page-footer.html" import page_footer %} {% block page_title %} - Enter the codes we’ve sent you – GOV.UK Notify + {% if form.email_code %} + Enter the codes we’ve sent you – GOV.UK Notify + {% else %} + Enter the code we’ve sent you – GOV.UK Notify + {% endif %} {% endblock %} {% block maincolumn_content %}
-

Enter the codes we’ve sent you

-

- We’ve sent you confirmation codes by email and text message. -

+ {% if form.email_code %} +

Enter the codes we’ve sent you

+ {% else %} +

Enter the code we’ve sent you

+ {% endif %} + {% if form.email_code %} +

+ We’ve sent you confirmation codes by email and text message. +

+ {% else %} +

+ We’ve sent you a confirmation code by text message. +

+ {% endif %}
+ {% if form.sms_code %} {{ textbox( - form.sms_code, - width='5em', - help_link=url_for('.check_and_resend_text_code'), - help_link_text='I haven’t received a text message' - ) }} - {{ textbox( - form.email_code, - width='5em', - help_link=url_for('.check_and_resend_email_code'), - help_link_text='I haven’t received an email' - ) }} + form.sms_code, + width='5em', + help_link=url_for('.check_and_resend_text_code'), + help_link_text='I haven’t received a text message' + ) }} + {% endif %} + {% if form.email_code %} + {{ textbox( + form.email_code, + width='5em', + help_link=url_for('.check_and_resend_email_code'), + help_link_text='I haven’t received an email' + ) }} + {% endif %} {{ page_footer("Continue") }}
diff --git a/config.py b/config.py index ef8ced69b..bb520a3ae 100644 --- a/config.py +++ b/config.py @@ -13,11 +13,7 @@ class Config(object): NOTIFY_APP_NAME = 'admin' NOTIFY_LOG_PATH = '/var/log/notify/application.log' - SQLALCHEMY_COMMIT_ON_TEARDOWN = False - SQLALCHEMY_RECORD_QUERIES = True - SQLALCHEMY_DATABASE_URI = 'postgresql://localhost/notifications_admin' MAX_FAILED_LOGIN_COUNT = 10 - PASS_SECRET_KEY = 'secret-key-unique-changeme' SESSION_COOKIE_NAME = 'notify_admin_session' SESSION_COOKIE_PATH = '/admin' @@ -67,7 +63,6 @@ class Development(Config): class Test(Development): - SQLALCHEMY_DATABASE_URI = 'postgresql://localhost/test_notifications_admin' WTF_CSRF_ENABLED = False diff --git a/requirements.txt b/requirements.txt index 85ed0e61d..c0d9bbd45 100644 --- a/requirements.txt +++ b/requirements.txt @@ -14,4 +14,4 @@ Pygments==2.0.2 git+https://github.com/alphagov/notifications-python-client.git@0.3.1#egg=notifications-python-client==0.3.1 -git+https://github.com/alphagov/notifications-utils.git@2.0.0#egg=notifications-utils==2.0.0 +git+https://github.com/alphagov/notifications-utils.git@3.0.0#egg=notifications-utils==3.0.0 diff --git a/tests/app/main/notify_client/test_sender.py b/tests/app/main/notify_client/test_sender.py deleted file mode 100644 index aef8d1f0c..000000000 --- a/tests/app/main/notify_client/test_sender.py +++ /dev/null @@ -1,29 +0,0 @@ -from itsdangerous import BadSignature -from pytest import fail - -from app.notify_client.sender import generate_token, check_token - - -def test_should_return_email_from_signed_token(app_): - email = 'email@something.com' - token = generate_token(email) - assert email == check_token(token) - - -def test_should_throw_exception_when_token_is_tampered_with(app_): - email = 'email@something.com' - token = generate_token(email) - try: - check_token(token + 'qerqwer') - fail() - except BadSignature: - pass - - -def test_return_none_when_token_is_expired(app_): - with app_.test_request_context(): - app_.config['TOKEN_MAX_AGE_SECONDS'] = -1000 - email = 'email@something.com' - token = generate_token(email) - assert check_token(token) is None - app_.config['TOKEN_MAX_AGE_SECONDS'] = 120000 diff --git a/tests/app/main/test_errorhandlers.py b/tests/app/main/test_errorhandlers.py new file mode 100644 index 000000000..f4853632f --- /dev/null +++ b/tests/app/main/test_errorhandlers.py @@ -0,0 +1,10 @@ +from bs4 import BeautifulSoup +from flask import url_for + + +def test_bad_url_returns_page_not_found(app_): + with app_.test_client() as client: + response = client.get('/bad_url') + assert response.status_code == 404 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Page could not be found' diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 3e48e1f73..b74142682 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,48 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard(app_, assert response.location == expected_redirect_location +def test_existing_user_with_no_permissions_accept_invite(app_, + mocker, + service_one, + api_user_active, + sample_invite, + mock_check_invite_token, + mock_get_user_by_email, + mock_add_user_to_service): + + expected_service = service_one['id'] + sample_invite['permissions'] = '' + expected_permissions = [] + mocker.patch('app.invite_api_client.accept_invite', return_value=sample_invite) + + with app_.test_request_context(): + with app_.test_client() as client: + + response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) + mock_add_user_to_service.assert_called_with(expected_service, api_user_active.id, expected_permissions) + + assert response.status_code == 302 + + +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, @@ -125,8 +169,7 @@ def test_cancelled_invited_user_accepts_invited_redirect_to_cancelled_invitation service_one, mocker, mock_get_user, - mock_get_service - ): + mock_get_service): with app_.test_request_context(): with app_.test_client() as client: cancelled_invitation = create_sample_invite(mocker, service_one, status='cancelled') @@ -188,6 +231,7 @@ def test_new_user_accept_invite_completes_new_registration_redirects_to_verify(a def test_new_invited_user_verifies_and_added_to_service(app_, service_one, sample_invite, + api_user_active, mock_check_invite_token, mock_dont_get_user_by_email, mock_register_user, @@ -217,8 +261,7 @@ def test_new_invited_user_verifies_and_added_to_service(app_, response = client.post(url_for('main.register_from_invite'), data=data) # that sends user on to verify - response = client.post(url_for('main.verify'), data={'sms_code': '12345', 'email_code': '23456'}, - follow_redirects=True) + response = client.post(url_for('main.verify'), data={'sms_code': '12345'}, follow_redirects=True) # when they post codes back to admin user should be added to # service and sent on to dash board @@ -226,8 +269,8 @@ def test_new_invited_user_verifies_and_added_to_service(app_, with client.session_transaction() as session: new_user_id = session['user_id'] mock_add_user_to_service.assert_called_with(data['service'], new_user_id, expected_permissions) - - mock_accept_invite.assert_called_with(data['service'], sample_invite['id']) + mock_accept_invite.assert_called_with(data['service'], sample_invite['id']) + mock_check_verify_code.assert_called_once_with(new_user_id, '12345', 'sms') page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') element = page.find('h2', class_='navigation-service-name').find('a') @@ -236,4 +279,4 @@ def test_new_invited_user_verifies_and_added_to_service(app_, assert service_link == '/services/{}/dashboard'.format(service_one['id']) flash_banner = page.find('div', class_='banner-default-with-tick').string.strip() - assert flash_banner == 'You have sucessfully accepted your invitation and been added to Test Service' + assert flash_banner == 'You have successfully accepted your invitation and been added to Test Service' diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index c5a4fc9e7..2b1e1c264 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -7,7 +7,6 @@ def test_get_should_render_add_service_template(app_, mock_login, mock_get_service, mock_get_services, - mock_get_user, mock_get_user_by_email): with app_.test_request_context(): with app_.test_client() as client: @@ -21,9 +20,7 @@ def test_should_add_service_and_redirect_to_next_page(app_, mock_login, mock_create_service, mock_get_services, - api_user_active, - mock_get_user, - mock_get_user_by_email): + api_user_active): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -32,7 +29,9 @@ def test_should_add_service_and_redirect_to_next_page(app_, data={'name': 'testing the post'}) assert response.status_code == 302 assert response.location == url_for('main.service_dashboard', service_id=101, _external=True) - assert mock_create_service.called + mock_create_service.asset_called_once_with('testing the post', False, + app_.config['DEFAULT_SERVICE_LIMIT'], + True, api_user_active.id) def test_should_return_form_errors_when_service_name_is_empty(app_, diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index a335ea8be..d0ed7e571 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -123,7 +123,7 @@ def test_should_show_page_for_inviting_user( client.login(api_user_active) response = client.get(url_for('main.invite_user', service_id=55555)) - assert 'Add a new team member' in response.get_data(as_text=True) + assert 'Invite a team member' in response.get_data(as_text=True) assert response.status_code == 200 @@ -277,6 +277,6 @@ def test_user_cant_invite_themselves( assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Add a new team member' + assert page.h1.string.strip() == 'Invite a team member' form_error = page.find('span', class_='error-message').string.strip() assert form_error == "You can't send an invitation to yourself" diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index d61513784..9849da99f 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -44,7 +44,7 @@ def test_process_register_creates_new_user(app_, assert mock_register_user.called -def test_process_register_returns_400_when_mobile_number_is_invalid(app_, +def test_process_register_returns_200_when_mobile_number_is_invalid(app_, mock_send_verify_code, mock_get_user_by_email_not_found, mock_login): diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 5115d4e73..44fc9a1ee 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -19,17 +19,16 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( mock_has_permissions ): - contents = 'phone number,name\n+44 123,test1\n+44 456,test2' - file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv') + contents = u'phone number,name\n+44 123,test1\n+44 456,test2' mocker.patch('app.main.views.send.s3download', return_value=contents) with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) - upload_data = {'file': file_data} response = client.post( url_for('main.send_messages', service_id=12345, template_id=54321), - data=upload_data, + data={'file': (BytesIO(contents.encode('utf-8')), 'invalid.csv')}, + content_type='multipart/form-data', follow_redirects=True ) assert response.status_code == 200 @@ -126,7 +125,27 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mocker.patch( 'app.main.views.send.s3download', - return_value='phone number\n+44 7700 900981\n+44 7700 900982\n+44 7700 900983\n+44 7700 900984\n+44 7700 900985\n+44 7700 900986' # noqa + return_value=""" + phone number + +44 7700 9009 01 + +44 7700 9009 02 + +44 7700 9009 03 + +44 7700 9009 04 + +44 7700 9009 05 + +44 7700 9009 06 + +44 7700 9009 07 + +44 7700 9009 08 + +44 7700 9009 09 + +44 7700 9009 10 + +44 7700 9009 11 + +44 7700 9009 12 + +44 7700 9009 13 + +44 7700 9009 14 + +44 7700 9009 15 + +44 7700 9009 99 + +44 7700 9009 99 + +44 7700 9009 99 + """ ) with app_.test_request_context(): @@ -134,22 +153,21 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( client.login(api_user_active) response = client.post( url_for('main.send_messages', service_id=12345, template_id=54321), - data={'file': (BytesIO(), 'valid.csv')}, + data={'file': (BytesIO(''.encode('utf-8')), 'valid.csv')}, + content_type='multipart/form-data', follow_redirects=True ) with client.session_transaction() as sess: assert int(sess['upload_data']['template_id']) == 54321 assert sess['upload_data']['original_file_name'] == 'valid.csv' - assert sess['upload_data']['notification_count'] == 6 + assert sess['upload_data']['notification_count'] == 18 content = response.get_data(as_text=True) assert response.status_code == 200 - assert '+44 7700 900981' in content - assert '+44 7700 900982' in content - assert '+44 7700 900983' in content - assert '+44 7700 900984' in content - assert '+44 7700 900985' in content - assert '1 more row not shown' in content + assert '+44 7700 9009 01' in content + assert '+44 7700 9009 15' in content + assert '+44 7700 9009 16' not in content + assert '3 rows not shown' in content def test_create_job_should_call_api( @@ -206,7 +224,14 @@ def test_check_messages_should_revalidate_file_when_uploading_file( mocker.patch( 'app.main.views.send.s3download', - return_value='phone number,name,,,\n++44 7700 900981,test1,,,\n+44 7700 900981,test2,,,\n ,,, \n ,,, \t \t \n' + return_value=""" + phone number,name,,, + ++44 7700 900981,test1,,, + +44 7700 900981,test2,,, + ,,, + ,,, \t \t + + """ ) with app_.test_request_context(): with app_.test_client() as client: @@ -214,10 +239,12 @@ def test_check_messages_should_revalidate_file_when_uploading_file( with client.session_transaction() as session: session['upload_data'] = {'original_file_name': 'invalid.csv', 'template_id': job_data['template'], - 'notification_count': job_data['notification_count']} + 'notification_count': job_data['notification_count'], + 'valid': True} response = client.post( url_for('main.check_messages', service_id=service_id, upload_id=job_data['id']), - data={'file': (BytesIO(), 'invalid.csv')}, + data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')}, + content_type='multipart/form-data', follow_redirects=True ) assert response.status_code == 200 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/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index ef8569338..c3cc669d1 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -58,18 +58,6 @@ def test_should_return_locked_out_true_when_user_is_locked(app_, assert 'Username or password is incorrect' in resp.get_data(as_text=True) -def test_should_return_active_user_is_false_if_user_is_inactive(app_, mock_get_user_by_email_inactive): - - with app_.test_request_context(): - response = app_.test_client().post( - url_for('main.sign_in'), data={ - 'email_address': 'inactive_user@example.gov.uk', - 'password': 'val1dPassw0rd!'}) - - assert response.status_code == 200 - assert 'Username or password is incorrect' in response.get_data(as_text=True) - - def test_should_return_200_when_user_does_not_exist(app_, mock_get_user_by_email_not_found): with app_.test_request_context(): response = app_.test_client().post( diff --git a/tests/app/main/views/test_sign_out.py b/tests/app/main/views/test_sign_out.py index f4d7019a8..a6b8e5fcb 100644 --- a/tests/app/main/views/test_sign_out.py +++ b/tests/app/main/views/test_sign_out.py @@ -19,8 +19,6 @@ def test_sign_out_user(app_, mock_login, mock_get_jobs): with app_.test_request_context(): - email = 'valid@example.gov.uk' - password = 'val1dPassw0rd!' with app_.test_client() as client: client.login(api_user_active) with client.session_transaction() as session: diff --git a/tests/app/main/views/test_verify.py b/tests/app/main/views/test_verify.py index 4b5d790da..d149ba362 100644 --- a/tests/app/main/views/test_verify.py +++ b/tests/app/main/views/test_verify.py @@ -1,8 +1,4 @@ -from flask import json, url_for -from app.main.dao import users_dao -from tests import create_test_api_user - -import pytest +from flask import url_for def test_should_return_verify_template(app_, @@ -67,3 +63,26 @@ def test_should_return_200_when_codes_are_wrong(app_, assert response.status_code == 200 resp_data = response.get_data(as_text=True) assert resp_data.count('Code not found') == 2 + + +def test_should_only_check_codes_in_validation_if_both_are_present(app_, + api_user_active, + mock_get_user, + mock_update_user, + mock_check_verify_code): + with app_.test_request_context(): + with app_.test_client() as client: + with client.session_transaction() as session: + session['user_details'] = {'email_address': api_user_active.email_address, 'id': api_user_active.id} + response = client.post(url_for('main.verify'), data={'sms_code': '12345'}) + assert response.status_code == 200 + assert not mock_check_verify_code.called + + response = client.post(url_for('main.verify'), data={'email_code': '12345'}) + assert response.status_code == 200 + assert not mock_check_verify_code.called + + response = client.post(url_for('main.verify'), data={'sms_code': '12345', 'email_code': '12345'}) + assert response.status_code == 302 + assert mock_check_verify_code.called + assert mock_check_verify_code.call_count == 2 diff --git a/tests/conftest.py b/tests/conftest.py index 93dce2265..9d5c88f39 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): @@ -67,7 +70,7 @@ def mock_create_service(mocker): service = service_json( 101, service_name, [user_id], limit=limit, active=active, restricted=restricted) - return {'data': service} + return service['id'] return mocker.patch( 'app.notifications_api_client.create_service', side_effect=_create) @@ -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" @@ -311,6 +332,7 @@ def mock_register_user(mocker, api_user_pending): @pytest.fixture(scope='function') def mock_get_user(mocker, api_user_active): def _get_user(id): + api_user_active.id = id return api_user_active return mocker.patch( 'app.user_api_client.get_user', side_effect=_get_user) @@ -380,12 +402,7 @@ def mock_get_user_by_email_locked(mocker, api_user_locked): @pytest.fixture(scope='function') def mock_get_user_by_email_inactive(mocker, api_user_pending): - - def _get_user(email_address): - api_user_pending._email_address = email_address - api_user_pending._is_locked = True - return api_user_pending - return mocker.patch('app.user_api_client.get_user_by_email', side_effect=_get_user) + return mocker.patch('app.user_api_client.get_user_by_email', return_value=api_user_pending) @pytest.fixture(scope='function')