From f6d98b63ea2f41e8c726a2ac285d32668b94bcee Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 10 Mar 2016 11:53:29 +0000 Subject: [PATCH] Refactored register_errorhandlers so that it handles HTTPError Remove most cases where we catch HTTPError --- app/__init__.py | 9 +- app/main/dao/services_dao.py | 13 +-- app/main/dao/templates_dao.py | 5 +- app/main/views/dashboard.py | 34 +++---- app/main/views/forgot_password.py | 8 +- app/main/views/invites.py | 45 ++++------ app/main/views/jobs.py | 124 +++++++++++--------------- app/main/views/manage_users.py | 3 - app/main/views/register.py | 17 +--- app/main/views/send.py | 10 +-- app/main/views/service_settings.py | 75 +++------------- app/main/views/verify.py | 10 +-- app/notify_client/user_api_client.py | 2 - tests/app/main/views/test_register.py | 2 +- 14 files changed, 110 insertions(+), 247 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 8db5ca6d1..dfebc8dd0 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -6,6 +6,7 @@ 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 notifications_python_client import HTTPError from werkzeug.exceptions import abort from pygments import highlight from pygments.lexers import JavascriptLexer @@ -176,10 +177,10 @@ def useful_headers_after_request(response): def register_errorhandlers(application): - def render_error(error): - # If a HTTPException, pull the `code` attribute; default to 500 + @application.errorhandler(HTTPError) + def render_http_error(error): error_code = getattr(error, 'code', 500) + if error_code not in [401, 404, 403, 500]: + error_code = 500 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) diff --git a/app/main/dao/services_dao.py b/app/main/dao/services_dao.py index 4104b0ff4..1e867801b 100644 --- a/app/main/dao/services_dao.py +++ b/app/main/dao/services_dao.py @@ -1,7 +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): @@ -26,15 +25,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..69bbd272f 100644 --- a/app/main/dao/templates_dao.py +++ b/app/main/dao/templates_dao.py @@ -22,10 +22,7 @@ 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 + abort(e.status_code) def delete_service_template(service_id, template_id): diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index e0c2a84ab..415a7c141 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -16,29 +16,19 @@ 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 sucessfully 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=list(reversed(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..3f079e96e 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 ) -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,23 @@ 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) + return render_template('views/cancelled-invitation.html', + from_user=from_user.name, + service_name=service['name']) - try: + existing_user = user_api_client.get_user_by_email(invited_user.email_address) + session['invited_user'] = invited_user.serialize() - 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 existing_user: - existing_user = user_api_client.get_user_by_email(invited_user.email_address) - session['invited_user'] = invited_user.serialize() - - 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..6d2982a73 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -20,89 +20,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 5a0a7f5c9..fbb5377cc 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 eaeeaf150..924fa1d55 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 ( @@ -56,17 +54,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..0254663c1 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 @@ -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=[ diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index a57696375..f11fe7bf9 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -13,8 +13,6 @@ from flask_login import ( current_user ) -from notifications_python_client.errors import HTTPError - from app.main.dao.services_dao import ( get_service_by_id, delete_service, @@ -31,13 +29,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,13 +42,7 @@ 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() @@ -74,13 +61,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): @@ -104,13 +85,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 +102,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 +118,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 +141,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 +157,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 +165,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/verify.py b/app/main/views/verify.py index 63cc1cb18..9f36d9012 100644 --- a/app/main/views/verify.py +++ b/app/main/views/verify.py @@ -2,12 +2,9 @@ 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 @@ -31,11 +28,6 @@ def verify(): 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'] 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/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index da83375c3..5a9133d81 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -43,7 +43,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):