From 8247f3d5686afd463e8c55eac63ed7300b477f7e Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 22 Feb 2016 14:14:03 +0000 Subject: [PATCH 01/35] Added subject to client calls for templates --- app/notify_client/api_client.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/app/notify_client/api_client.py b/app/notify_client/api_client.py index f5f72298a..6d2a2016a 100644 --- a/app/notify_client/api_client.py +++ b/app/notify_client/api_client.py @@ -70,7 +70,7 @@ class NotificationsAdminAPIClient(NotificationsAPIClient): endpoint = "/service/{0}".format(service_id) return self.put(endpoint, data) - def create_service_template(self, name, type_, content, service_id): + def create_service_template(self, name, type_, content, service_id, subject=None): """ Create a service template. """ @@ -80,10 +80,14 @@ class NotificationsAdminAPIClient(NotificationsAPIClient): "content": content, "service": service_id } + if subject: + data.update({ + 'subject': subject + }) endpoint = "/service/{0}/template".format(service_id) return self.post(endpoint, data) - def update_service_template(self, id_, name, type_, content, service_id): + def update_service_template(self, id_, name, type_, content, service_id, subject=None): """ Update a service template. """ @@ -94,8 +98,12 @@ class NotificationsAdminAPIClient(NotificationsAPIClient): 'content': content, 'service': service_id } + if subject: + data.update({ + 'subject': subject + }) endpoint = "/service/{0}/template/{1}".format(service_id, id_) - return self.put(endpoint, data) + return self.post(endpoint, data) def get_service_template(self, service_id, template_id, *params): """ From c6de605311ded7a11d2d9e2d78fc6db1f3ab108a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Feb 2016 14:45:13 +0000 Subject: [PATCH 02/35] Add basic flow for adding email _or_ sms templates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Templates now have: - a type (email or sms) - a subject (if they are email templates) We don’t want two completely separate view files for email and SMS, because they would have an enormous amount of repetition. So this commit adds - different templates for SMS and email templates - different form objects for SMS and email templates …and wires them up. --- app/main/dao/templates_dao.py | 6 +- app/main/forms.py | 9 ++- app/main/views/email.py | 0 app/main/views/index.py | 12 --- app/main/views/sms.py | 21 +++--- app/main/views/templates.py | 74 ++++++++++--------- app/templates/main_nav.html | 4 +- .../views/choose-email-template.html | 38 ++++++++++ app/templates/views/choose-sms-template.html | 2 +- ...template.html => edit-email-template.html} | 7 +- app/templates/views/edit-sms-template.html | 43 +++++++++++ app/templates/views/send-email.html | 17 ----- tests/app/main/views/test_sms.py | 2 +- tests/app/main/views/test_templates.py | 32 ++------ 14 files changed, 160 insertions(+), 107 deletions(-) create mode 100644 app/main/views/email.py create mode 100644 app/templates/views/choose-email-template.html rename app/templates/views/{edit-template.html => edit-email-template.html} (80%) create mode 100644 app/templates/views/edit-sms-template.html delete mode 100644 app/templates/views/send-email.html diff --git a/app/main/dao/templates_dao.py b/app/main/dao/templates_dao.py index d34f2b187..b7ce664f8 100644 --- a/app/main/dao/templates_dao.py +++ b/app/main/dao/templates_dao.py @@ -4,12 +4,12 @@ from app.utils import BrowsableItem from notifications_python_client.errors import HTTPError -def insert_service_template(name, content, service_id): +def insert_service_template(name, content, service_id, subject=None): return notifications_api_client.create_service_template( - name, 'sms', content, service_id) + name, 'sms' if subject is None else 'email', content, service_id, subject) -def update_service_template(id_, name, content, service_id): +def update_service_template(id_, name, content, service_id, subject=None): return notifications_api_client.update_service_template( id_, name, 'sms', content, service_id) diff --git a/app/main/forms.py b/app/main/forms.py index 20b007514..ea2e1cd69 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -188,7 +188,7 @@ class ConfirmPasswordForm(Form): raise ValidationError('Invalid password') -class TemplateForm(Form): +class SMSTemplateForm(Form): name = StringField( u'Template name', validators=[DataRequired(message="Template name cannot be empty")]) @@ -198,6 +198,13 @@ class TemplateForm(Form): validators=[DataRequired(message="Template content cannot be empty")]) +class EmailTemplateForm(SMSTemplateForm): + + subject = StringField( + u'Subject', + validators=[DataRequired(message="Subject cannot be empty")]) + + class ForgotPasswordForm(Form): email_address = email_address() diff --git a/app/main/views/email.py b/app/main/views/email.py new file mode 100644 index 000000000..e69de29bb diff --git a/app/main/views/index.py b/app/main/views/index.py index 1200cf65b..8335284cd 100644 --- a/app/main/views/index.py +++ b/app/main/views/index.py @@ -22,15 +22,3 @@ def register_from_invite(): @login_required def verify_mobile(): return render_template('views/verify-mobile.html') - - -@main.route("/services//send-email") -@login_required -def send_email(service_id): - return render_template('views/send-email.html', service_id=service_id) - - -@main.route("/services//check-email") -@login_required -def check_email(service_id): - return render_template('views/check-email.html') diff --git a/app/main/views/sms.py b/app/main/views/sms.py index 42340cbaa..63b947218 100644 --- a/app/main/views/sms.py +++ b/app/main/views/sms.py @@ -35,8 +35,10 @@ from app.utils import ( ) -@main.route("/services//sms/send", methods=['GET']) -def choose_sms_template(service_id): +@main.route("/services//send/", methods=['GET']) +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: @@ -44,21 +46,18 @@ def choose_sms_template(service_id): abort(404) else: raise e - print("="*80) - print(jobs) - print(len(jobs)) - print(bool(len(jobs))) return render_template( - 'views/choose-sms-template.html', + 'views/choose-{}-template.html'.format(template_type), templates=[ Template(template) for template in templates_dao.get_service_templates(service_id)['data'] + if template['template_type'] == template_type ], has_jobs=len(jobs), service_id=service_id ) -@main.route("/services//sms/send/", methods=['GET', 'POST']) +@main.route("/services//send/", methods=['GET', 'POST']) @login_required def send_sms(service_id, template_id): @@ -91,7 +90,7 @@ def send_sms(service_id, template_id): ) -@main.route("/services//sms/send/.csv", methods=['GET']) +@main.route("/services//send/.csv", methods=['GET']) @login_required def get_example_csv(service_id, template_id): template = templates_dao.get_service_template_or_404(service_id, template_id)['data'] @@ -104,7 +103,7 @@ def get_example_csv(service_id, template_id): return(output.getvalue(), 200, {'Content-Type': 'text/csv; charset=utf-8'}) -@main.route("/services//sms/send//to-self", methods=['GET']) +@main.route("/services//send//to-self", methods=['GET']) @login_required def send_sms_to_self(service_id, template_id): template = templates_dao.get_service_template_or_404(service_id, template_id)['data'] @@ -126,7 +125,7 @@ def send_sms_to_self(service_id, template_id): upload_id=upload_id)) -@main.route("/services//sms/check/", +@main.route("/services//check/", methods=['GET', 'POST']) @login_required def check_sms(service_id, upload_id): diff --git a/app/main/views/templates.py b/app/main/views/templates.py index be1001113..b291e1b8c 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -5,45 +5,44 @@ from notifications_python_client.errors import HTTPError from utils.template import Template from app.main import main -from app.main.forms import TemplateForm +from app.main.forms import SMSTemplateForm, EmailTemplateForm from app import job_api_client -from app.main.dao.services_dao import get_service_by_id +from app.main.dao.services_dao import get_service_by_id_or_404 from app.main.dao import templates_dao as tdao from app.main.dao import services_dao as sdao -@main.route("/services//templates") +form_objects = { + 'email': EmailTemplateForm, + 'sms': SMSTemplateForm +} + + +@main.route("/services//templates/add-", methods=['GET', 'POST']) @login_required -def manage_service_templates(service_id): - return redirect(url_for( - '.choose_sms_template', - service_id=service_id - )) +def add_service_template(service_id, template_type): + service = sdao.get_service_by_id_or_404(service_id) -@main.route("/services//templates/add", methods=['GET', 'POST']) -@login_required -def add_service_template(service_id): - try: - service = sdao.get_service_by_id(service_id)['data'] - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + if template_type not in ['sms', 'email']: + abort(404) - form = TemplateForm() + form = form_objects[template_type]() if form.validate_on_submit(): tdao.insert_service_template( - form.name.data, form.template_content.data, service_id) - return redirect(url_for( - '.choose_sms_template', service_id=service_id)) + form.name.data, form.template_content.data, service_id, form.subject.data or None + ) + return redirect( + url_for('.choose_template', service_id=service_id, template_type=template_type) + ) + return render_template( - 'views/edit-template.html', - h1='Add a text message template', + 'views/edit-{}-template.html'.format(template_type), form=form, - service_id=service_id) + template_type=template_type, + service_id=service_id + ) @main.route("/services//templates/", methods=['GET', 'POST']) @@ -51,20 +50,25 @@ def add_service_template(service_id): def edit_service_template(service_id, template_id): template = tdao.get_service_template_or_404(service_id, template_id)['data'] template['template_content'] = template['content'] - form = TemplateForm(**template) + form = form_objects[template['template_type']](**template) if form.validate_on_submit(): tdao.update_service_template( template_id, form.name.data, form.template_content.data, service_id) - return redirect(url_for('.choose_sms_template', service_id=service_id)) + return redirect(url_for( + '.choose_template', + service_id=service_id, + template_type=template['template_type'] + )) return render_template( - 'views/edit-template.html', - h1='Edit template', + 'views/edit-{}-template.html'.format(template['template_type']), form=form, service_id=service_id, - template_id=template_id) + template_id=template_id, + template_type=template['template_type'] + ) @main.route("/services//templates//delete", methods=['GET', 'POST']) @@ -74,13 +78,17 @@ def delete_service_template(service_id, template_id): if request.method == 'POST': tdao.delete_service_template(service_id, template_id) - return redirect(url_for('.manage_service_templates', service_id=service_id)) + return redirect(url_for( + '.choose_template', + service_id=service_id, + template_type=template['template_type'] + )) template['template_content'] = template['content'] - form = TemplateForm(**template) + form = form_objects[template['template_type']](**template) flash('Are you sure you want to delete ‘{}’?'.format(form.name.data), 'delete') return render_template( - 'views/edit-template.html', + 'views/edit-{}-template.html'.format(template['template_type']), h1='Edit template', form=form, service_id=service_id, diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 4a379f3d8..a08a24a4a 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -3,8 +3,8 @@ {{ session.get('service_name', 'Service') }}
  • Manage team
    • diff --git a/app/templates/views/choose-email-template.html b/app/templates/views/choose-email-template.html new file mode 100644 index 000000000..06ed1dec6 --- /dev/null +++ b/app/templates/views/choose-email-template.html @@ -0,0 +1,38 @@ +{% extends "withnav_template.html" %} +{% from "components/email-message.html" import email_message %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/textbox.html" import textbox %} + +{% block page_title %} + Send emails – GOV.UK Notify +{% endblock %} + +{% block maincolumn_content %} + +

    Send emails

    + +
    + + {% if templates %} +
    + {% for template in templates %} +
    + {{ email_message(template.subject, template.formatted_as_markup, name=template.name) }} +
    +
    +
    + Add recipients + Send yourself a test + Edit template +
    +
    + {% endfor %} +
    + {% endif %} + +

    + Add a new template +

    + +
    +{% endblock %} diff --git a/app/templates/views/choose-sms-template.html b/app/templates/views/choose-sms-template.html index 221fa781b..b83217a7c 100644 --- a/app/templates/views/choose-sms-template.html +++ b/app/templates/views/choose-sms-template.html @@ -40,7 +40,7 @@ {% endif %}

    - Add a new template + Add a new template

    diff --git a/app/templates/views/edit-template.html b/app/templates/views/edit-email-template.html similarity index 80% rename from app/templates/views/edit-template.html rename to app/templates/views/edit-email-template.html index 4720a2a61..b0c983f65 100644 --- a/app/templates/views/edit-template.html +++ b/app/templates/views/edit-email-template.html @@ -8,12 +8,15 @@ {% block maincolumn_content %} -

    {{ h1 }}

    +

    Edit email template

    {{ textbox(form.name, width='1-1') }} + {% if 'email' == template_type %} + {{ textbox(form.subject, width='1-1') }} + {% endif %}
    @@ -31,7 +34,7 @@ '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_sms_template', service_id=service_id), + back_link=url_for('.choose_template', template_type=template_type, service_id=service_id), back_link_text='Cancel' ) }} diff --git a/app/templates/views/edit-sms-template.html b/app/templates/views/edit-sms-template.html new file mode 100644 index 000000000..8c8d5e1c5 --- /dev/null +++ b/app/templates/views/edit-sms-template.html @@ -0,0 +1,43 @@ +{% extends "withnav_template.html" %} +{% from "components/textbox.html" import textbox %} +{% from "components/page-footer.html" import page_footer %} + +{% block page_title %} + {{ h1 }} – GOV.UK Notify +{% endblock %} + +{% block maincolumn_content %} + +

    Edit text message template

    + +
    +
    +
    + {{ textbox(form.name, width='1-1') }} + {% if 'email' == template_type %} + {{ textbox(form.subject, width='1-1') }} + {% endif %} +
    +
    +
    +
    + {{ textbox(form.template_content, highlight_tags=True, width='1-1') }} +
    +
    + +
    +
    + {{ 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' + ) }} +
    + + +{% endblock %} diff --git a/app/templates/views/send-email.html b/app/templates/views/send-email.html deleted file mode 100644 index 3fe253f32..000000000 --- a/app/templates/views/send-email.html +++ /dev/null @@ -1,17 +0,0 @@ -{% extends "withnav_template.html" %} - -{% block page_title %} -Send email – GOV.UK Notify -{% endblock %} - -{% block maincolumn_content %} - -

    Send email

    - -

    This page will be where we construct email messages

    - -

    - Continue -

    - -{% endblock %} diff --git a/tests/app/main/views/test_sms.py b/tests/app/main/views/test_sms.py index 6f9656e25..c48aeed26 100644 --- a/tests/app/main/views/test_sms.py +++ b/tests/app/main/views/test_sms.py @@ -14,7 +14,7 @@ def test_choose_sms_template(app_, with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) - response = client.get(url_for('main.choose_sms_template', service_id=12345)) + response = client.get(url_for('main.choose_template', template_type='sms', service_id=12345)) assert response.status_code == 200 content = response.get_data(as_text=True) diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index cecc59353..1ec43d502 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -4,24 +4,6 @@ import uuid from flask import url_for -def test_should_return_list_of_all_templates(app_, - api_user_active, - mock_get_service_templates, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_get_jobs): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) - service_id = str(uuid.uuid4()) - response = client.get(url_for( - '.manage_service_templates', service_id=service_id), follow_redirects=True) - - assert response.status_code == 200 - mock_get_service_templates.assert_called_with(service_id) - - def test_should_show_page_for_one_templates(app_, api_user_active, mock_get_service_template, @@ -62,8 +44,9 @@ def test_should_redirect_when_saving_a_template(app_, data = { 'id': template_id, 'name': name, - "template_content": content, - "service": service_id + 'template_content': content, + 'type': 'sms', + 'service': service_id } response = client.post(url_for( '.edit_service_template', @@ -72,7 +55,7 @@ def test_should_redirect_when_saving_a_template(app_, assert response.status_code == 302 assert response.location == url_for( - '.choose_sms_template', service_id=service_id, _external=True) + '.choose_template', service_id=service_id, template_type='sms', _external=True) mock_update_service_template.assert_called_with( template_id, name, 'sms', content, service_id) @@ -127,12 +110,13 @@ def test_should_redirect_when_deleting_a_template(app_, response = client.post(url_for( '.delete_service_template', service_id=service_id, - template_id=template_id), data=data) + template_id=template_id + ), data=data) assert response.status_code == 302 assert response.location == url_for( - '.manage_service_templates', - service_id=service_id, _external=True) + '.choose_template', + service_id=service_id, template_type=type_, _external=True) mock_get_service_template.assert_called_with( service_id, template_id) mock_delete_service_template.assert_called_with( From aaa631737151ef5ba2d2738c1a0b6a3822a8bdec Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Feb 2016 16:46:18 +0000 Subject: [PATCH 03/35] Rename SMS to send Because this view is no longer just for sending SMS messages, it and its associated tests should be renamed, so `send_sms.py` becomes `send.py`, etc. --- app/main/__init__.py | 2 +- app/main/views/{sms.py => send.py} | 0 tests/app/main/views/{test_sms.py => test_send.py} | 0 3 files changed, 1 insertion(+), 1 deletion(-) rename app/main/views/{sms.py => send.py} (100%) rename tests/app/main/views/{test_sms.py => test_send.py} (100%) diff --git a/app/main/__init__.py b/app/main/__init__.py index fbfd2562a..ec9691d24 100644 --- a/app/main/__init__.py +++ b/app/main/__init__.py @@ -3,7 +3,7 @@ from flask import Blueprint main = Blueprint('main', __name__) from app.main.views import ( - index, sign_in, sign_out, register, two_factor, verify, sms, add_service, + index, sign_in, sign_out, register, two_factor, verify, send, add_service, code_not_received, jobs, dashboard, templates, service_settings, forgot_password, new_password, styleguide, user_profile, choose_service, api_keys, manage_users ) diff --git a/app/main/views/sms.py b/app/main/views/send.py similarity index 100% rename from app/main/views/sms.py rename to app/main/views/send.py diff --git a/tests/app/main/views/test_sms.py b/tests/app/main/views/test_send.py similarity index 100% rename from tests/app/main/views/test_sms.py rename to tests/app/main/views/test_send.py From 1e4692287668aa11ab1d1d825bc77754ca2f8810 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Feb 2016 17:17:18 +0000 Subject: [PATCH 04/35] Make send the send flow generic This commit parameterises all methods in the send view so that they can send either emails or SMS messages. It works out what kind of message it is sending from the `template_type` property of the template object. This means that the `Template` util class needs to know about these properties, which means that this commit depends on: https://github.com/alphagov/notifications-utils/pull/2 This commit does _not_ add tests for sending emails. The existing tests for sending SMS still pass, but actually sending emails is outside the scope of this story. --- app/assets/stylesheets/components/banner.scss | 2 +- app/main/dao/templates_dao.py | 8 +- app/main/views/send.py | 64 +++++---- app/main/views/templates.py | 7 +- app/templates/views/check-sms.html | 2 +- .../views/choose-email-template.html | 2 - app/templates/views/choose-sms-template.html | 4 +- .../views/{send-sms.html => send.html} | 19 ++- app/templates/views/service_dashboard.html | 4 +- app/utils.py | 20 +++ requirements.txt | 2 +- tests/app/main/views/test_send.py | 124 +++++++++++------- tests/conftest.py | 19 ++- 13 files changed, 174 insertions(+), 103 deletions(-) rename app/templates/views/{send-sms.html => send.html} (57%) diff --git a/app/assets/stylesheets/components/banner.scss b/app/assets/stylesheets/components/banner.scss index c62bfe2c6..77b706779 100644 --- a/app/assets/stylesheets/components/banner.scss +++ b/app/assets/stylesheets/components/banner.scss @@ -71,7 +71,7 @@ color: $text-colour; background-image: file-url('icon-important-2x.png'); background-size: 34px 34px; - background-position: 0 0px; + background-position: 0 0; background-repeat: no-repeat; padding: 7px 0 5px 50px; } diff --git a/app/main/dao/templates_dao.py b/app/main/dao/templates_dao.py index b7ce664f8..7d07b7d0e 100644 --- a/app/main/dao/templates_dao.py +++ b/app/main/dao/templates_dao.py @@ -4,14 +4,14 @@ from app.utils import BrowsableItem from notifications_python_client.errors import HTTPError -def insert_service_template(name, content, service_id, subject=None): +def insert_service_template(name, type_, content, service_id, subject=None): return notifications_api_client.create_service_template( - name, 'sms' if subject is None else 'email', content, service_id, subject) + name, type_, content, service_id, subject) -def update_service_template(id_, name, content, service_id, subject=None): +def update_service_template(id_, name, type_, content, service_id, subject=None): return notifications_api_client.update_service_template( - id_, name, 'sms', content, service_id) + id_, name, type_, content, service_id) def get_service_templates(service_id): diff --git a/app/main/views/send.py b/app/main/views/send.py index 63b947218..91a41fc5f 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -28,15 +28,21 @@ from app.main.uploader import ( s3download ) from app.main.dao import templates_dao +from app.main.dao import services_dao from app import job_api_client -from app.utils import ( - validate_phone_number, - InvalidPhoneError -) +from app.utils import validate_recipient, InvalidPhoneError, InvalidEmailError + +first_column_header = { + 'email': 'email', + 'sms': 'phone' +} @main.route("/services//send/", methods=['GET']) def choose_template(service_id, template_type): + + services_dao.get_service_by_id_or_404(service_id) + if template_type not in ['email', 'sms']: abort(404) try: @@ -59,7 +65,7 @@ def choose_template(service_id, template_type): @main.route("/services//send/", methods=['GET', 'POST']) @login_required -def send_sms(service_id, template_id): +def send_messages(service_id, template_id): form = CsvUploadForm() if form.validate_on_submit(): @@ -69,23 +75,25 @@ def send_sms(service_id, template_id): upload_id = str(uuid.uuid4()) s3upload(upload_id, service_id, filedata, current_app.config['AWS_REGION']) session['upload_data'] = {"template_id": template_id, "original_file_name": filedata['file_name']} - return redirect(url_for('.check_sms', + return redirect(url_for('.check_messages', service_id=service_id, upload_id=upload_id)) except ValueError as e: flash('There was a problem uploading: {}'.format(csv_file.filename)) flash(str(e)) - return redirect(url_for('.send_sms', service_id=service_id, template_id=template_id)) + return redirect(url_for('.send_messages', service_id=service_id, template_id=template_id)) + service = services_dao.get_service_by_id_or_404(service_id) template = Template( templates_dao.get_service_template_or_404(service_id, template_id)['data'] ) return render_template( - 'views/send-sms.html', + 'views/send.html', template=template, - column_headers=['phone'] + template.placeholders_as_markup, + column_headers=[first_column_header[template.template_type]] + template.placeholders_as_markup, form=form, + service=service, service_id=service_id ) @@ -97,7 +105,7 @@ def get_example_csv(service_id, template_id): placeholders = list(Template(template).placeholders) output = io.StringIO() writer = csv.writer(output) - writer.writerow(['phone'] + placeholders) + writer.writerow([first_column_header[template['template_type']]] + placeholders) writer.writerow([current_user.mobile_number] + ["test {}".format(header) for header in placeholders]) return(output.getvalue(), 200, {'Content-Type': 'text/csv; charset=utf-8'}) @@ -105,12 +113,12 @@ def get_example_csv(service_id, template_id): @main.route("/services//send//to-self", methods=['GET']) @login_required -def send_sms_to_self(service_id, template_id): +def send_message_to_self(service_id, template_id): template = templates_dao.get_service_template_or_404(service_id, template_id)['data'] placeholders = list(Template(template).placeholders) output = io.StringIO() writer = csv.writer(output) - writer.writerow(['phone'] + placeholders) + writer.writerow([first_column_header[template['template_type']]] + placeholders) writer.writerow([current_user.mobile_number] + ["test {}".format(header) for header in placeholders]) filedata = { 'file_name': 'Test run', @@ -120,7 +128,7 @@ def send_sms_to_self(service_id, template_id): s3upload(upload_id, service_id, filedata, current_app.config['AWS_REGION']) session['upload_data'] = {"template_id": template_id, "original_file_name": filedata['file_name']} - return redirect(url_for('.check_sms', + return redirect(url_for('.check_messages', service_id=service_id, upload_id=upload_id)) @@ -128,27 +136,29 @@ def send_sms_to_self(service_id, template_id): @main.route("/services//check/", methods=['GET', 'POST']) @login_required -def check_sms(service_id, upload_id): +def check_messages(service_id, upload_id): + + upload_data = session['upload_data'] + template_id = upload_data.get('template_id') if request.method == 'GET': contents = s3download(service_id, upload_id) if not contents: flash('There was a problem reading your upload file') - upload_data = session['upload_data'] - template_id = upload_data.get('template_id') raw_template = templates_dao.get_service_template_or_404(service_id, template_id)['data'] + recipient_type = first_column_header[raw_template['template_type']] upload_result = _get_rows(contents, raw_template) session['upload_data']['notification_count'] = len(upload_result['rows']) template = Template( raw_template, values=upload_result['rows'][0] if upload_result['valid'] else {}, - drop_values={'phone'} + drop_values={recipient_type} ) return render_template( 'views/check-sms.html', upload_result=upload_result, template=template, - column_headers=['phone number'] + list( + column_headers=[recipient_type] + list( template.placeholders if upload_result['valid'] else template.placeholders_as_markup ), original_file_name=upload_data.get('original_file_name'), @@ -156,9 +166,7 @@ def check_sms(service_id, upload_id): form=CsvUploadForm() ) elif request.method == 'POST': - upload_data = session['upload_data'] original_file_name = upload_data.get('original_file_name') - template_id = upload_data.get('template_id') notification_count = upload_data.get('notification_count') session.pop('upload_data') try: @@ -170,9 +178,9 @@ def check_sms(service_id, upload_id): raise e flash('We’ve started sending your messages', 'default_with_tick') - return redirect(url_for('main.view_job', - service_id=service_id, - job_id=upload_id)) + return redirect( + url_for('main.view_job', service_id=service_id, job_id=upload_id) + ) def _get_filedata(file): @@ -195,8 +203,12 @@ def _get_rows(contents, raw_template): for row in reader: rows.append(row) try: - validate_phone_number(row['phone']) - Template(raw_template, values=row, drop_values={'phone'}).replaced - except (InvalidPhoneError, NeededByTemplateError, NoPlaceholderForDataError): + recipient_column = first_column_header[raw_template['template_type']] + validate_recipient( + row[recipient_column], + template_type=raw_template['template_type'] + ) + Template(raw_template, values=row, drop_values={recipient_column}).replaced + except (InvalidEmailError, InvalidPhoneError, NeededByTemplateError, NoPlaceholderForDataError): valid = False return {"valid": valid, "rows": rows} diff --git a/app/main/views/templates.py b/app/main/views/templates.py index b291e1b8c..01f3711dd 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -31,7 +31,7 @@ def add_service_template(service_id, template_type): if form.validate_on_submit(): tdao.insert_service_template( - form.name.data, form.template_content.data, service_id, form.subject.data or None + form.name.data, template['template_type'], form.template_content.data, service_id, form.subject.data or None ) return redirect( url_for('.choose_template', service_id=service_id, template_type=template_type) @@ -54,8 +54,9 @@ def edit_service_template(service_id, template_id): if form.validate_on_submit(): tdao.update_service_template( - template_id, form.name.data, - form.template_content.data, service_id) + template_id, form.name.data, template['template_type'], + form.template_content.data, service_id + ) return redirect(url_for( '.choose_template', service_id=service_id, diff --git a/app/templates/views/check-sms.html b/app/templates/views/check-sms.html index 4b1354847..e0de38dca 100644 --- a/app/templates/views/check-sms.html +++ b/app/templates/views/check-sms.html @@ -42,7 +42,7 @@
    - Back + Back
    {% else %} {{file_upload(form.file, button_text='Upload a CSV file')}} diff --git a/app/templates/views/choose-email-template.html b/app/templates/views/choose-email-template.html index 06ed1dec6..fb5fe2b1f 100644 --- a/app/templates/views/choose-email-template.html +++ b/app/templates/views/choose-email-template.html @@ -21,8 +21,6 @@
    - Add recipients - Send yourself a test Edit template
    diff --git a/app/templates/views/choose-sms-template.html b/app/templates/views/choose-sms-template.html index b83217a7c..5508ca4fb 100644 --- a/app/templates/views/choose-sms-template.html +++ b/app/templates/views/choose-sms-template.html @@ -30,8 +30,8 @@
    - Add recipients - Send yourself a test + Add recipients + Send yourself a test Edit template
    diff --git a/app/templates/views/send-sms.html b/app/templates/views/send.html similarity index 57% rename from app/templates/views/send-sms.html rename to app/templates/views/send.html index c8a91388d..8aef83078 100644 --- a/app/templates/views/send-sms.html +++ b/app/templates/views/send.html @@ -1,5 +1,6 @@ {% extends "withnav_template.html" %} {% from "components/sms-message.html" import sms_message %} +{% from "components/email-message.html" import email_message %} {% from "components/page-footer.html" import page_footer %} {% from "components/file-upload.html" import file_upload %} {% from "components/table.html" import list_table, field %} @@ -14,14 +15,22 @@
    - - {{ sms_message(template.formatted_as_markup) }} - + {% if 'sms' == template.template_type %} + {{ sms_message(template.formatted_as_markup) }} + {% elif 'email' == template.template_type %} + {{ email_message( + template.subject, + template.formatted_as_markup, + from_address='{}@notifications.service.gov.uk'.format(service.email_from), + from_name=service.name + ) }} + {% endif %} {{ banner( - 'You can upload real data, but we’ll only send to your mobile number until you request to go live'|safe, + 'You can upload real data, but we’ll only send to your mobile number until you request to go live'.format( + url_for('.service_request_to_go_live', service_id=service_id) + )|safe, 'info' )}} -
    diff --git a/app/templates/views/service_dashboard.html b/app/templates/views/service_dashboard.html index 045a90695..b17c935ba 100644 --- a/app/templates/views/service_dashboard.html +++ b/app/templates/views/service_dashboard.html @@ -36,7 +36,7 @@ """.format( url_for(".add_service_template", service_id=service_id), - url_for(".choose_sms_template", service_id=service_id) + url_for(".choose_template", service_id=service_id, template_type="sms") )|safe, subhead='Get started', type="tip" @@ -46,7 +46,7 @@ """ Send yourself a text message """.format( - url_for(".choose_sms_template", service_id=service_id) + url_for(".choose_template", service_id=service_id, template_type="sms") )|safe, subhead='Next step', type="tip" diff --git a/app/utils.py b/app/utils.py index f814f4b81..9468cba77 100644 --- a/app/utils.py +++ b/app/utils.py @@ -1,3 +1,5 @@ +import re + from functools import wraps from flask import abort @@ -28,6 +30,11 @@ class BrowsableItem(object): pass +class InvalidEmailError(Exception): + def __init__(self, message): + self.message = message + + class InvalidPhoneError(Exception): def __init__(self, message): self.message = message @@ -74,6 +81,19 @@ def format_phone_number(number): return '+447{}{}{}'.format(*re.findall('...', number)) +def validate_email_address(email_address): + if re.match(r"(^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+\.[a-zA-Z0-9-.]+$)", email_address): + return + raise InvalidEmailError('Not a valid email address') + + +def validate_recipient(recipient, template_type): + return { + 'email': validate_email_address, + 'sms': validate_phone_number + }[template_type](recipient) + + def user_has_permissions(*permissions): def wrap(func): @wraps(func) diff --git a/requirements.txt b/requirements.txt index 49dceaccf..af95431af 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.2.5#egg=notifications-python-client==0.2.7 -git+https://github.com/alphagov/notifications-utils.git@0.1.0#egg=notifications-utils==0.1.0 +git+https://github.com/alphagov/notifications-utils.git@0.1.0#egg=notifications-utils==0.1.1 diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index c48aeed26..7e9e8dc1a 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1,42 +1,56 @@ from io import BytesIO from flask import url_for +import pytest import moto +template_types = ['email', 'sms'] -def test_choose_sms_template(app_, - api_user_active, - mock_login, - mock_get_user, - mock_check_verify_code, - mock_get_service_templates, - mock_get_jobs): + +@pytest.mark.parametrize("template_type", template_types) +def test_choose_template( + template_type, + app_, + api_user_active, + mock_login, + mock_get_user, + mock_get_service, + mock_check_verify_code, + mock_get_service_templates, + mock_get_jobs +): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) - response = client.get(url_for('main.choose_template', template_type='sms', service_id=12345)) + response = client.get(url_for('main.choose_template', template_type=template_type, service_id=12345)) assert response.status_code == 200 content = response.get_data(as_text=True) - assert 'template_one' in content - assert 'template one content' in content - assert 'template_two' in content - assert 'template two content' in content + assert '{}_template_one'.format(template_type) in content + assert '{} template one content'.format(template_type) in content + assert '{}_template_two'.format(template_type) in content + assert '{} template two content'.format(template_type) in content -def test_upload_empty_csvfile_returns_to_upload_page(app_, - api_user_active, - mock_login, - mock_get_user, - mock_get_service_templates, - mock_check_verify_code, - mock_get_service_template): +def test_upload_empty_csvfile_returns_to_upload_page( + app_, + api_user_active, + mock_login, + mock_get_user, + mock_get_service, + mock_get_service_templates, + mock_check_verify_code, + mock_get_service_template +): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) upload_data = {'file': (BytesIO(''.encode('utf-8')), 'emtpy.csv')} - response = client.post(url_for('main.send_sms', service_id=12345, template_id=54321), - data=upload_data, follow_redirects=True) + response = client.post( + url_for('main.send_messages', service_id=12345, template_id=54321), + data=upload_data, + follow_redirects=True + ) assert response.status_code == 200 content = response.get_data(as_text=True) @@ -44,13 +58,15 @@ def test_upload_empty_csvfile_returns_to_upload_page(app_, @moto.mock_s3 -def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors(app_, - mocker, - api_user_active, - mock_login, - mock_get_user, - mock_get_user_by_email, - mock_get_service_template): +def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors( + app_, + mocker, + api_user_active, + mock_login, + mock_get_user, + mock_get_user_by_email, + mock_get_service_template +): contents = 'phone\n+44 123\n+44 456' file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv') @@ -59,9 +75,11 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors(app_, with app_.test_client() as client: client.login(api_user_active) upload_data = {'file': file_data} - response = client.post(url_for('main.send_sms', service_id=12345, template_id=54321), - data=upload_data, - follow_redirects=True) + response = client.post( + url_for('main.send_messages', service_id=12345, template_id=54321), + data=upload_data, + follow_redirects=True + ) assert response.status_code == 200 content = response.get_data(as_text=True) assert 'Your CSV file contained missing or invalid data' in content @@ -85,7 +103,7 @@ def test_send_test_message_to_self( with app_.test_client() as client: client.login(api_user_active) response = client.get( - url_for('main.send_sms_to_self', service_id=12345, template_id=54321), + url_for('main.send_message_to_self', service_id=12345, template_id=54321), follow_redirects=True ) assert response.status_code == 200 @@ -118,13 +136,15 @@ def test_download_example_csv( @moto.mock_s3 -def test_upload_csvfile_with_valid_phone_shows_all_numbers(app_, - mocker, - api_user_active, - mock_login, - mock_get_user, - mock_get_user_by_email, - mock_get_service_template): +def test_upload_csvfile_with_valid_phone_shows_all_numbers( + app_, + mocker, + api_user_active, + mock_login, + mock_get_user, + mock_get_user_by_email, + mock_get_service_template +): contents = 'phone\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 @@ -134,7 +154,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers(app_, with app_.test_client() as client: client.login(api_user_active) upload_data = {'file': file_data} - response = client.post(url_for('main.send_sms', service_id=12345, template_id=54321), + response = client.post(url_for('main.send_messages', service_id=12345, template_id=54321), data=upload_data, follow_redirects=True) with client.session_transaction() as sess: @@ -154,16 +174,18 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers(app_, @moto.mock_s3 -def test_create_job_should_call_api(app_, - service_one, - api_user_active, - mock_get_user, - mock_get_user_by_email, - mock_login, - job_data, - mock_create_job, - mock_get_job, - mock_get_service_template): +def test_create_job_should_call_api( + app_, + service_one, + api_user_active, + mock_get_user, + mock_get_user_by_email, + mock_login, + job_data, + mock_create_job, + mock_get_job, + mock_get_service_template +): service_id = service_one['id'] job_id = job_data['id'] @@ -178,7 +200,7 @@ def test_create_job_should_call_api(app_, session['upload_data'] = {'original_file_name': original_file_name, 'template_id': template_id, 'notification_count': notification_count} - url = url_for('main.check_sms', service_id=service_one['id'], upload_id=job_id) + url = url_for('main.check_messages', service_id=service_one['id'], upload_id=job_id) response = client.post(url, data=job_data, follow_redirects=True) assert response.status_code == 200 diff --git a/tests/conftest.py b/tests/conftest.py index 308ee42b7..3c1de5a87 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -163,11 +163,20 @@ def mock_update_service_template(mocker): @pytest.fixture(scope='function') def mock_get_service_templates(mocker): def _create(service_id): - template_one = template_json( - 1, "template_one", "sms", "template one content", service_id) - template_two = template_json( - 2, "template_two", "sms", "template two content", service_id) - return {'data': [template_one, template_two]} + return {'data': [ + template_json( + 1, "sms_template_one", "sms", "sms template one content", service_id + ), + template_json( + 2, "sms_template_two", "sms", "sms template two content", service_id + ), + template_json( + 3, "email_template_one", "email", "email template one content", service_id + ), + template_json( + 4, "email_template_two", "email", "email template two content", service_id + ) + ]} return mocker.patch( 'app.notifications_api_client.get_service_templates', From bc1899e8c08fa09dbafca0f91f03754786e600a0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 23 Feb 2016 14:58:49 +0000 Subject: [PATCH 05/35] Make email pattern work in new context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The email pattern looked a bit shonky when displayed in a narrower column. This commit fixes it by making the email’s metadata (eg subject, from) into a table, which it sort of is. This means that it is more flexible about the size of container in which it sits. --- .../stylesheets/components/email-message.scss | 23 ++++++++-- .../stylesheets/components/sms-message.scss | 2 +- app/main/views/templates.py | 2 +- app/templates/components/email-message.html | 44 +++++++++---------- .../views/choose-email-template.html | 2 +- 5 files changed, 44 insertions(+), 29 deletions(-) diff --git a/app/assets/stylesheets/components/email-message.scss b/app/assets/stylesheets/components/email-message.scss index b4771841a..860b3d99e 100644 --- a/app/assets/stylesheets/components/email-message.scss +++ b/app/assets/stylesheets/components/email-message.scss @@ -7,9 +7,26 @@ margin: 20px 0 10px 0; } - &-subject, - &-from { - margin: 10px 0; + &-meta { + + @include core-19; + margin: 0; + + td, + th { + @include core-19; + border-bottom: 0; + border-top: 1px solid $border-colour; + } + + th { + color: $secondary-text-colour; + } + + td { + width: 99%; + } + } &-from { diff --git a/app/assets/stylesheets/components/sms-message.scss b/app/assets/stylesheets/components/sms-message.scss index 898d04c24..cb26b296f 100644 --- a/app/assets/stylesheets/components/sms-message.scss +++ b/app/assets/stylesheets/components/sms-message.scss @@ -40,7 +40,7 @@ .sms-message-use-links { @include copy-19; - margin-top: 55px; + margin-top: 52px; a { diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 01f3711dd..09eea1906 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -31,7 +31,7 @@ def add_service_template(service_id, template_type): if form.validate_on_submit(): tdao.insert_service_template( - form.name.data, template['template_type'], form.template_content.data, service_id, form.subject.data or None + form.name.data, template_type, form.template_content.data, service_id, form.subject.data or None ) return redirect( url_for('.choose_template', service_id=service_id, template_type=template_type) diff --git a/app/templates/components/email-message.html b/app/templates/components/email-message.html index b619b16fa..4b0047651 100644 --- a/app/templates/components/email-message.html +++ b/app/templates/components/email-message.html @@ -9,29 +9,27 @@ {% endif %}