From 6e1267826fc04301dd3035adeb2f2e681144c7ec Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 24 Feb 2016 15:45:46 +0000 Subject: [PATCH 01/20] Removed print statement --- app/main/views/sms.py | 1 - app/notify_client/job_api_client.py | 2 -- 2 files changed, 3 deletions(-) diff --git a/app/main/views/sms.py b/app/main/views/sms.py index a135643f7..0b4815d26 100644 --- a/app/main/views/sms.py +++ b/app/main/views/sms.py @@ -127,7 +127,6 @@ def check_sms(service_id, upload_id): template_id = upload_data.get('template_id') raw_template = templates_dao.get_service_template_or_404(service_id, template_id)['data'] upload_result = _get_rows(contents, raw_template) - print(upload_result) template = Template( raw_template, values=upload_result['rows'][0] if upload_result['valid'] else {}, diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index 7b7be15c9..17745a109 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -1,4 +1,3 @@ -import uuid from notifications_python_client.base import BaseAPIClient @@ -23,7 +22,6 @@ class JobApiClient(BaseAPIClient): def create_job(self, job_id, service_id, template_id, original_file_name): data = { "id": job_id, - "service": service_id, "template": template_id, "original_file_name": original_file_name, "bucket_name": "service-{}-notify".format(service_id), From 87391f2aa19919e3fcf5875189a75c093052f7aa Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 25 Feb 2016 11:30:09 +0000 Subject: [PATCH 02/20] Renamed header of CSV to be 'to' - matches API --- app/main/views/send.py | 28 ++++++------------- app/templates/views/check-sms.html | 6 ++-- .../app/main/notify_client/test_job_client.py | 1 - 3 files changed, 11 insertions(+), 24 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 91a41fc5f..e9dfeea44 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -1,9 +1,6 @@ import csv import io import uuid -import botocore - -from datetime import date from flask import ( request, @@ -17,7 +14,6 @@ from flask import ( ) from flask_login import login_required, current_user -from werkzeug import secure_filename from notifications_python_client.errors import HTTPError from utils.template import Template, NeededByTemplateError, NoPlaceholderForDataError @@ -32,11 +28,6 @@ from app.main.dao import services_dao from app import job_api_client 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): @@ -91,7 +82,7 @@ def send_messages(service_id, template_id): return render_template( 'views/send.html', template=template, - column_headers=[first_column_header[template.template_type]] + template.placeholders_as_markup, + column_headers=['to'] + template.placeholders_as_markup, form=form, service=service, service_id=service_id @@ -105,10 +96,9 @@ def get_example_csv(service_id, template_id): placeholders = list(Template(template).placeholders) output = io.StringIO() writer = csv.writer(output) - writer.writerow([first_column_header[template['template_type']]] + placeholders) + writer.writerow(['to'] + 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'}) + return output.getvalue(), 200, {'Content-Type': 'text/csv; charset=utf-8'} @main.route("/services//send//to-self", methods=['GET']) @@ -118,7 +108,7 @@ def send_message_to_self(service_id, template_id): placeholders = list(Template(template).placeholders) output = io.StringIO() writer = csv.writer(output) - writer.writerow([first_column_header[template['template_type']]] + placeholders) + writer.writerow(['to'] + placeholders) writer.writerow([current_user.mobile_number] + ["test {}".format(header) for header in placeholders]) filedata = { 'file_name': 'Test run', @@ -146,19 +136,18 @@ def check_messages(service_id, upload_id): if not contents: flash('There was a problem reading your upload file') 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={recipient_type} + drop_values={'to'} ) return render_template( 'views/check-sms.html', upload_result=upload_result, template=template, - column_headers=[recipient_type] + list( + column_headers=['to'] + list( template.placeholders if upload_result['valid'] else template.placeholders_as_markup ), original_file_name=upload_data.get('original_file_name'), @@ -203,12 +192,11 @@ def _get_rows(contents, raw_template): for row in reader: rows.append(row) try: - recipient_column = first_column_header[raw_template['template_type']] validate_recipient( - row[recipient_column], + row['to'], template_type=raw_template['template_type'] ) - Template(raw_template, values=row, drop_values={recipient_column}).replaced + Template(raw_template, values=row, drop_values={'to'}).replaced except (InvalidEmailError, InvalidPhoneError, NeededByTemplateError, NoPlaceholderForDataError): valid = False return {"valid": valid, "rows": rows} diff --git a/app/templates/views/check-sms.html b/app/templates/views/check-sms.html index e0de38dca..38c793639 100644 --- a/app/templates/views/check-sms.html +++ b/app/templates/views/check-sms.html @@ -53,13 +53,13 @@ caption=original_file_name, field_headings=column_headers ) %} - {% if item.phone|valid_phone_number %} + {% if item.to|valid_phone_number %} {% call field() %} - {{ item.phone }} + {{ item.to }} {% endcall %} {% else %} {% call field(status='missing') %} - {{ item.phone }} + {{ item.to }} {% endcall %} {% endif %} {% for column in template.placeholders %} diff --git a/tests/app/main/notify_client/test_job_client.py b/tests/app/main/notify_client/test_job_client.py index e59b2927a..1eb2be38d 100644 --- a/tests/app/main/notify_client/test_job_client.py +++ b/tests/app/main/notify_client/test_job_client.py @@ -11,7 +11,6 @@ def test_client_creates_job_data_correctly(mocker): expected_data = { "id": job_id, - "service": service_id, "template": template_id, "original_file_name": original_file_name, "bucket_name": "service-{}-notify".format(service_id), From 8d81c4d9846a3ebcf82ea4758f73223203bdd0fc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 Feb 2016 17:34:49 +0000 Subject: [PATCH 03/20] Stub out the send letters page --- app/main/views/send.py | 7 +++++++ app/templates/main_nav.html | 1 + app/templates/views/letters.html | 17 +++++++++++++++++ 3 files changed, 25 insertions(+) create mode 100644 app/templates/views/letters.html diff --git a/app/main/views/send.py b/app/main/views/send.py index e9dfeea44..e8631ce84 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -29,6 +29,13 @@ from app import job_api_client from app.utils import validate_recipient, InvalidPhoneError, InvalidEmailError +@main.route("/services//send/letters", methods=['GET']) +def letters_stub(service_id): + return render_template( + 'views/letters.html', service_id=service_id + ) + + @main.route("/services//send/", methods=['GET']) def choose_template(service_id, template_type): diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index a08a24a4a..f6eef7ae6 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -5,6 +5,7 @@
  • Manage team
    • diff --git a/app/templates/views/letters.html b/app/templates/views/letters.html new file mode 100644 index 000000000..1086f1d50 --- /dev/null +++ b/app/templates/views/letters.html @@ -0,0 +1,17 @@ +{% extends "withnav_template.html" %} + +{% block page_title %} + Send letters – GOV.UK Notify +{% endblock %} + +{% block maincolumn_content %} + +

    + Send letters +

    + +

    + This page is where you would go to send letters. +

    + +{% endblock %} From d53dfec3fd474bbabdceb63eebf01155ea10f1b4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Feb 2016 11:46:52 +0000 Subject: [PATCH 04/20] Temporarily remove SASS linting Trying to install gulp-sass-lint fails because some of its dependencies are not met: > npm install gulp-sass-lint > npm ERR! Darwin 15.3.0 > npm ERR! argv "/usr/local/bin/node" "/usr/local/bin/npm" "install" "gulp-sass-lint" > npm ERR! node v5.7.0 > npm ERR! npm v3.7.5 > > npm ERR! No compatible version found: lodash.upperfirst@^3.0.0 > npm ERR! Valid install targets: > npm ERR! 4.1.2, 4.1.1, 4.1.0, 4.0.2, 4.0.1, 4.0.0 > npm ERR! This commit removes it until we can work out what the problem is. --- gulpfile.babel.js | 9 +++++---- package.json | 4 +--- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/gulpfile.babel.js b/gulpfile.babel.js index 030c5eeb7..6f2356c80 100644 --- a/gulpfile.babel.js +++ b/gulpfile.babel.js @@ -103,10 +103,11 @@ gulp.task('watchForChanges', function() { }); gulp.task('lint:sass', () => gulp - .src(paths.src + 'stylesheets/**/*.scss') - .pipe(plugins.sassLint()) - .pipe(plugins.sassLint.format(stylish)) - .pipe(plugins.sassLint.failOnError()) +// Removed until gulp-sass-lint installs cleanly +// .src(paths.src + 'stylesheets/**/*.scss') +// .pipe(plugins.sassLint()) +// .pipe(plugins.sassLint.format(stylish)) +// .pipe(plugins.sassLint.failOnError()) ); gulp.task('lint:js', () => gulp diff --git a/package.json b/package.json index 267f7d7a7..a15a60448 100644 --- a/package.json +++ b/package.json @@ -29,9 +29,8 @@ "gulp-base64": "0.1.3", "gulp-concat": "2.6.0", "gulp-include": "2.1.0", - "gulp-jquery": "1.1.1", "gulp-load-plugins": "1.1.0", - "gulp-sass": "2.1.1", + "gulp-sass": "2.2.0", "gulp-uglify": "1.5.1", "jquery": "1.11.2", "query-command-supported": "1.0.0" @@ -39,7 +38,6 @@ "devDependencies": { "gulp-css-url-adjuster": "0.2.3", "gulp-jshint": "2.0.0", - "gulp-sass-lint": "1.1.1", "jshint": "2.9.1", "jshint-stylish": "2.1.0" } From a51d92c87a7f0d18f1223cefd111b06e47e4a902 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 Feb 2016 16:37:39 +0000 Subject: [PATCH 05/20] Consolidate page templates for emails + texts Use one page template for each of: - choosing a message template - adding recipients - checking an email message - looking at a job This commit consolidates the two templates into one, and adds logic to show the SMS message pattern or the email message pattern depending on the type of template. It also gives email templates a bit more width, because the body and the from address tend to be quite long. --- app/main/views/jobs.py | 5 ++- app/main/views/send.py | 19 +++++++--- app/templates/views/check-email.html | 18 ---------- .../views/{check-sms.html => check.html} | 28 +++++++++------ .../views/choose-email-template.html | 36 ------------------- ...sms-template.html => choose-template.html} | 17 ++++++--- app/templates/views/job.html | 21 +++++++---- app/templates/views/send.html | 25 +++++++------ tests/app/main/views/test_jobs.py | 1 + 9 files changed, 80 insertions(+), 90 deletions(-) delete mode 100644 app/templates/views/check-email.html rename app/templates/views/{check-sms.html => check.html} (74%) delete mode 100644 app/templates/views/choose-email-template.html rename app/templates/views/{choose-sms-template.html => choose-template.html} (69%) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index dd5e45e04..15c21e65c 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -13,6 +13,7 @@ from utils.template import Template from app import job_api_client from app.main import main from app.main.dao import templates_dao +from app.main.dao import services_dao now = time.strftime('%H:%M') @@ -37,6 +38,7 @@ def view_jobs(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'] messages = [] @@ -58,7 +60,8 @@ def view_job(service_id, job_id): template=Template( templates_dao.get_service_template_or_404(service_id, job['template'])['data'] ), - service_id=service_id + service_id=service_id, + service=service ) except HTTPError as e: if e.status_code == 404: diff --git a/app/main/views/send.py b/app/main/views/send.py index e9dfeea44..67947df2b 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -28,11 +28,16 @@ from app.main.dao import services_dao from app import job_api_client from app.utils import validate_recipient, InvalidPhoneError, InvalidEmailError +page_headings = { + 'email': 'Send emails', + 'sms': 'Send text messages' +} + @main.route("/services//send/", methods=['GET']) def choose_template(service_id, template_type): - services_dao.get_service_by_id_or_404(service_id) + service = services_dao.get_service_by_id_or_404(service_id) if template_type not in ['email', 'sms']: abort(404) @@ -44,11 +49,14 @@ def choose_template(service_id, template_type): else: raise e return render_template( - 'views/choose-{}-template.html'.format(template_type), + 'views/choose-template.html', templates=[ Template(template) for template in templates_dao.get_service_templates(service_id)['data'] if template['template_type'] == template_type ], + template_type=template_type, + page_heading=page_headings[template_type], + service=service, has_jobs=len(jobs), service_id=service_id ) @@ -130,6 +138,7 @@ def check_messages(service_id, upload_id): upload_data = session['upload_data'] template_id = upload_data.get('template_id') + service = services_dao.get_service_by_id_or_404(service_id) if request.method == 'GET': contents = s3download(service_id, upload_id) @@ -144,14 +153,16 @@ def check_messages(service_id, upload_id): drop_values={'to'} ) return render_template( - 'views/check-sms.html', + 'views/check.html', upload_result=upload_result, template=template, + page_heading=page_headings[template.template_type], column_headers=['to'] + list( template.placeholders if upload_result['valid'] else template.placeholders_as_markup ), original_file_name=upload_data.get('original_file_name'), service_id=service_id, + service=service, form=CsvUploadForm() ) elif request.method == 'POST': @@ -193,7 +204,7 @@ def _get_rows(contents, raw_template): rows.append(row) try: validate_recipient( - row['to'], + row.get('to', ''), template_type=raw_template['template_type'] ) Template(raw_template, values=row, drop_values={'to'}).replaced diff --git a/app/templates/views/check-email.html b/app/templates/views/check-email.html deleted file mode 100644 index 3ca4830bf..000000000 --- a/app/templates/views/check-email.html +++ /dev/null @@ -1,18 +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 check the email messages we're about to send

    - -

    - Send email messages -

    - - -{% endblock %} diff --git a/app/templates/views/check-sms.html b/app/templates/views/check.html similarity index 74% rename from app/templates/views/check-sms.html rename to app/templates/views/check.html index 38c793639..48967ca7a 100644 --- a/app/templates/views/check-sms.html +++ b/app/templates/views/check.html @@ -1,4 +1,5 @@ {% extends "withnav_template.html" %} +{% from "components/email-message.html" import email_message %} {% from "components/sms-message.html" import sms_message %} {% from "components/table.html" import list_table, field %} {% from "components/placeholder.html" import placeholder %} @@ -6,7 +7,7 @@ {% from "components/page-footer.html" import page_footer %} {% block page_title %} - Check and confirm – GOV.UK Notify + {{ "Check and confirm" if upload_result.valid else page_heading }} – GOV.UK Notify {% endblock %} {% block maincolumn_content %} @@ -25,18 +26,23 @@ {% endif %}

    - {{ "Check and confirm" if upload_result.valid else "Send text messages" }} + {{ "Check and confirm" if upload_result.valid else page_heading }}

    -
    -
    - {% if template.missing_data or template.additional_data %} - {{ sms_message(template.formatted_as_markup)}} - {% else %} - {{ sms_message(template.replaced)}} - {% endif %} + {% if 'email' == template.template_type %} + {{ email_message( + template.subject, + template, + from_address='{}@notifications.service.gov.uk'.format(service.email_from), + from_name=service.name + )}} + {% elif 'sms' == template.template_type %} +
    +
    + {{ sms_message(template)}} +
    -
    + {% endif %} {% if upload_result.valid %}
    @@ -53,7 +59,7 @@ caption=original_file_name, field_headings=column_headers ) %} - {% if item.to|valid_phone_number %} + {% if item.to or ''|valid_phone_number %} {% call field() %} {{ item.to }} {% endcall %} diff --git a/app/templates/views/choose-email-template.html b/app/templates/views/choose-email-template.html deleted file mode 100644 index 5452a5da8..000000000 --- a/app/templates/views/choose-email-template.html +++ /dev/null @@ -1,36 +0,0 @@ -{% 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) }} -
    -
    -
    - Edit template -
    -
    - {% endfor %} -
    - {% endif %} - -

    - Add a new template -

    - -
    -{% endblock %} diff --git a/app/templates/views/choose-sms-template.html b/app/templates/views/choose-template.html similarity index 69% rename from app/templates/views/choose-sms-template.html rename to app/templates/views/choose-template.html index 5508ca4fb..7fb87b827 100644 --- a/app/templates/views/choose-sms-template.html +++ b/app/templates/views/choose-template.html @@ -1,15 +1,16 @@ {% extends "withnav_template.html" %} +{% from "components/email-message.html" import email_message %} {% from "components/sms-message.html" import sms_message %} {% from "components/page-footer.html" import page_footer %} {% from "components/textbox.html" import textbox %} {% block page_title %} - Send text messages – GOV.UK Notify + {{ page_heading }} – GOV.UK Notify {% endblock %} {% block maincolumn_content %} -

    Send text messages

    +

    {{ page_heading }}

    @@ -26,7 +27,15 @@
    {% for template in templates %}
    - {{ sms_message(template.formatted_as_markup, name=template.name) }} + {% 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 %}
    @@ -40,7 +49,7 @@ {% endif %}

    - Add a new template + Add a new template

    diff --git a/app/templates/views/job.html b/app/templates/views/job.html index 0c038141c..ae60fed13 100644 --- a/app/templates/views/job.html +++ b/app/templates/views/job.html @@ -14,13 +14,22 @@ {{ uploaded_file_name }} -
    -
    - {{ sms_message( - template.formatted_as_markup, - )}} + {% if 'sms' == template.template_type %} +
    +
    + {{ sms_message( + template, + )}} +
    -
    + {% elif 'email' == template.template_type %} + {{ email_message( + template.subject, + template, + from_address='{}@notifications.service.gov.uk'.format(service.email_from), + from_name=service.name + )}} + {% endif %}

    Started {{ uploaded_file_time|format_datetime }} diff --git a/app/templates/views/send.html b/app/templates/views/send.html index 8aef83078..f98c819b1 100644 --- a/app/templates/views/send.html +++ b/app/templates/views/send.html @@ -13,18 +13,23 @@

    Add recipients

    + {% if 'sms' == template.template_type %} +
    +
    + {{ sms_message(template) }} +
    +
    + {% elif 'email' == template.template_type %} + {{ email_message( + template.subject, + template, + from_address='{}@notifications.service.gov.uk'.format(service.email_from), + from_name=service.name + ) }} + {% endif %} +
    - {% 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'.format( url_for('.service_request_to_go_live', service_id=service_id) diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 98f99e514..c3147555f 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -27,6 +27,7 @@ def test_should_show_page_for_one_job(app_, mock_login, mock_get_user, mock_get_user_by_email, + mock_get_service, mock_get_service_template, job_data, mock_get_job): From 70ef2f5a6f554a392b6448b86981e9d5bef1e688 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 Feb 2016 17:16:02 +0000 Subject: [PATCH 06/20] Fix example CSV for email templates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It should populate the CSV file with the user’s email address, not their phone number. --- app/main/views/send.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 67947df2b..dc52d55cf 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -105,7 +105,12 @@ def get_example_csv(service_id, template_id): output = io.StringIO() writer = csv.writer(output) writer.writerow(['to'] + placeholders) - writer.writerow([current_user.mobile_number] + ["test {}".format(header) for header in placeholders]) + writer.writerow([ + { + 'email': current_user.email_address, + 'sms': current_user.mobile_number + }[template['template_type']] + ] + ["test {}".format(header) for header in placeholders]) return output.getvalue(), 200, {'Content-Type': 'text/csv; charset=utf-8'} From 2f91848a42b00c48037951cab094250ea4ca4a66 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Feb 2016 09:33:45 +0000 Subject: [PATCH 07/20] Prefix text messages with service name Brings in: https://github.com/alphagov/notifications-utils/pull/4 --- app/main/views/jobs.py | 3 ++- app/main/views/send.py | 11 ++++++++--- app/templates/views/check.html | 6 ++++-- app/templates/views/send.html | 4 ++-- requirements.txt | 2 +- 5 files changed, 17 insertions(+), 9 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 15c21e65c..9469fa810 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -58,7 +58,8 @@ def view_job(service_id, job_id): uploaded_file_name=job['original_file_name'], uploaded_file_time=job['created_at'], template=Template( - templates_dao.get_service_template_or_404(service_id, job['template'])['data'] + templates_dao.get_service_template_or_404(service_id, job['template'])['data'], + prefix=service['name'] ), service_id=service_id, service=service diff --git a/app/main/views/send.py b/app/main/views/send.py index dc52d55cf..cfdbd97d3 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -51,7 +51,10 @@ def choose_template(service_id, template_type): return render_template( 'views/choose-template.html', templates=[ - Template(template) for template in templates_dao.get_service_templates(service_id)['data'] + Template( + template, + prefix=service['name'] + ) for template in templates_dao.get_service_templates(service_id)['data'] if template['template_type'] == template_type ], template_type=template_type, @@ -84,7 +87,8 @@ def send_messages(service_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'] + templates_dao.get_service_template_or_404(service_id, template_id)['data'], + prefix=service['name'] ) return render_template( @@ -155,7 +159,8 @@ def check_messages(service_id, upload_id): template = Template( raw_template, values=upload_result['rows'][0] if upload_result['valid'] else {}, - drop_values={'to'} + drop_values={'to'}, + prefix=service['name'] ) return render_template( 'views/check.html', diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 48967ca7a..315bc32eb 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -32,14 +32,16 @@ {% if 'email' == template.template_type %} {{ email_message( template.subject, - template, + template.replaced if upload_result.valid else template.formatted_as_markup, from_address='{}@notifications.service.gov.uk'.format(service.email_from), from_name=service.name )}} {% elif 'sms' == template.template_type %}
    - {{ sms_message(template)}} + {{ sms_message( + template.replaced if upload_result.valid else template.formatted_as_markup + )}}
    {% endif %} diff --git a/app/templates/views/send.html b/app/templates/views/send.html index f98c819b1..25eda7364 100644 --- a/app/templates/views/send.html +++ b/app/templates/views/send.html @@ -16,13 +16,13 @@ {% if 'sms' == template.template_type %}
    - {{ sms_message(template) }} + {{ sms_message(template.formatted_as_markup) }}
    {% elif 'email' == template.template_type %} {{ email_message( template.subject, - template, + template.formatted_as_markup, from_address='{}@notifications.service.gov.uk'.format(service.email_from), from_name=service.name ) }} diff --git a/requirements.txt b/requirements.txt index cbb153dd6..2c746a497 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.8#egg=notifications-python-client==0.2.8 -git+https://github.com/alphagov/notifications-utils.git@0.1.1#egg=notifications-utils==0.1.1 +git+https://github.com/alphagov/notifications-utils.git@0.1.2#egg=notifications-utils==0.1.2 From 04e7aac99c8f4e18e32ff914cc7172de2c728808 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Feb 2016 09:35:07 +0000 Subject: [PATCH 08/20] Column headers on the check page should always be blue --- app/main/views/send.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index cfdbd97d3..07f50b5e2 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -167,9 +167,7 @@ def check_messages(service_id, upload_id): upload_result=upload_result, template=template, page_heading=page_headings[template.template_type], - column_headers=['to'] + list( - template.placeholders if upload_result['valid'] else template.placeholders_as_markup - ), + column_headers=['to'] + list(template.placeholders_as_markup), original_file_name=upload_data.get('original_file_name'), service_id=service_id, service=service, From d7bad32ac32b8b36b1a0c38e576b880d472a3d3c Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Fri, 26 Feb 2016 10:54:06 +0000 Subject: [PATCH 09/20] Removed moto mocks and mock s3 upload and download directly. Also updated csv reading to use correct module and do more work to remove empty fields and trailing commas. --- app/main/views/send.py | 31 +++++++++--- tests/app/main/views/test_send.py | 81 +++++++++++++++++++------------ tests/conftest.py | 7 +++ 3 files changed, 81 insertions(+), 38 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 82a9aa310..856403105 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -79,7 +79,7 @@ def send_messages(service_id, template_id): form = CsvUploadForm() if form.validate_on_submit(): try: - csv_file = form.file.data + csv_file = form.file filedata = _get_filedata(csv_file) upload_id = str(uuid.uuid4()) s3upload(upload_id, service_id, filedata, current_app.config['AWS_REGION']) @@ -88,7 +88,7 @@ def send_messages(service_id, template_id): service_id=service_id, upload_id=upload_id)) except ValueError as e: - flash('There was a problem uploading: {}'.format(csv_file.filename)) + flash('There was a problem uploading: {}'.format(csv_file.data.filename)) flash(str(e)) return redirect(url_for('.send_messages', service_id=service_id, template_id=template_id)) @@ -199,17 +199,34 @@ def check_messages(service_id, upload_id): def _get_filedata(file): - lines = file.read().decode('utf-8').splitlines() - if len(lines) < 2: # must be at least header and one line - message = 'The file {} contained no data'.format(file.filename) + import itertools + reader = csv.reader( + file.data.getvalue().decode('utf-8').splitlines(), + quoting=csv.QUOTE_NONE, + skipinitialspace=True + ) + lines = [] + for row in reader: + non_empties = itertools.dropwhile(lambda x: x.strip() == '', row) + has_content = [] + for item in non_empties: + has_content.append(item) + if has_content: + lines.append(row) + + if len(lines) < 2: # must be header row and at least one data row + message = 'The file {} contained no data'.format(file.data.filename) raise ValueError(message) - return {'file_name': file.filename, 'data': lines} + + content_lines = [] + for row in lines: + content_lines.append(','.join(row).rstrip(',')) + return {'file_name': file.data.filename, 'data': content_lines} def _get_rows(contents, raw_template): reader = csv.DictReader( contents.split('\n'), - lineterminator='\n', quoting=csv.QUOTE_NONE, skipinitialspace=True ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index d59da4593..11cd63de8 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1,8 +1,8 @@ +import pytest + from io import BytesIO from flask import url_for - -import pytest -import moto +from unittest.mock import ANY template_types = ['email', 'sms'] @@ -57,17 +57,18 @@ def test_upload_empty_csvfile_returns_to_upload_page( assert 'The file emtpy.csv contained no data' in content -@pytest.mark.skipif(True, reason='Errors on travis') -@moto.mock_s3 def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors( app_, api_user_active, + mocker, mock_login, - mock_get_service_template + mock_get_service_template, + mock_s3_upload ): - contents = 'phone\n+44 123\n+44 456' + contents = 'to,name\n+44 123,test1\n+44 456,test2' file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv') + mocker.patch('app.main.views.send.s3download', return_value=contents) with app_.test_request_context(): with app_.test_client() as client: @@ -86,18 +87,49 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors( assert 'Upload a CSV file' in content -@pytest.mark.skipif(True, reason='Errors on travis') -@moto.mock_s3 +def test_upload_csvfile_removes_empty_lines_and_trailing_commas( + app_, + api_user_active, + mocker, + mock_login, + mock_get_service_template, + mock_s3_upload +): + + contents = 'to,name,,,\n++44 7700 900981,test1,,,\n+44 7700 900981,test2,,,\n ,,, \n ,,, \t \t \n' + file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv') + + expected_data = {'data': ['to,name', '++44 7700 900981,test1', '+44 7700 900981,test2'], + 'file_name': 'invalid.csv'} + + mocker.patch('app.main.views.send.s3download', + return_value='to,name\n++44 7700 900981,test1\n+44 7700 900981,test2') + + 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, + follow_redirects=True + ) + assert response.status_code == 200 + mock_s3_upload.assert_called_with(ANY, '12345', expected_data, 'eu-west-1') + + def test_send_test_message_to_self( app_, mocker, api_user_active, mock_login, - mock_get_user, - mock_get_user_by_email, - mock_get_service_template + mock_get_service_template, + mock_s3_upload ): + expected_data = {'data': ['to', '+4412341234'], 'file_name': 'Test run'} + mocker.patch('app.main.views.send.s3download', return_value='to\r\n+4412341234') + with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -106,20 +138,14 @@ def test_send_test_message_to_self( follow_redirects=True ) assert response.status_code == 200 - content = response.get_data(as_text=True) - assert 'Test run' in content - assert '+4412341234' in content + mock_s3_upload.assert_called_with(ANY, '12345', expected_data, 'eu-west-1') -@pytest.mark.skipif(True, reason='Errors on travis') -@moto.mock_s3 def test_download_example_csv( app_, mocker, api_user_active, mock_login, - mock_get_user, - mock_get_user_by_email, mock_get_service_template ): @@ -131,25 +157,22 @@ def test_download_example_csv( follow_redirects=True ) assert response.status_code == 200 - assert response.get_data(as_text=True) == 'phone\r\n+4412341234\r\n' + assert response.get_data(as_text=True) == 'to\r\n+4412341234\r\n' assert 'text/csv' in response.headers['Content-Type'] -@pytest.mark.skipif(True, reason='Errors on travis') -@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 + mock_get_service_template, + mock_s3_upload ): - 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 - + contents = 'to\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 file_data = (BytesIO(contents.encode('utf-8')), 'valid.csv') + mocker.patch('app.main.views.send.s3download', return_value=contents) with app_.test_request_context(): with app_.test_client() as client: @@ -174,14 +197,10 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( assert '+44 7700 900986' in content -@pytest.mark.skipif(True, reason='Errors on travis') -@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, diff --git a/tests/conftest.py b/tests/conftest.py index ff8e7659f..b9622bf9e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -526,3 +526,10 @@ def mock_get_users_by_service(mocker): 'failed_login_count': 0}] return data return mocker.patch('app.user_api_client.get_users_for_service', side_effect=_get_users_for_service, autospec=True) + + +@pytest.fixture(scope='function') +def mock_s3_upload(mocker): + def _upload(upload_id, service_id, filedata, region): + pass + return mocker.patch('app.main.views.send.s3upload', side_effect=_upload) From 73cf7935666caf1ce7c7e60a670a7494a10be276 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Fri, 26 Feb 2016 12:11:55 +0000 Subject: [PATCH 10/20] Merge from master --- tests/app/main/views/test_send.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 11cd63de8..e638a5481 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -62,6 +62,7 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors( api_user_active, mocker, mock_login, + mock_get_service, mock_get_service_template, mock_s3_upload ): @@ -92,6 +93,7 @@ def test_upload_csvfile_removes_empty_lines_and_trailing_commas( api_user_active, mocker, mock_login, + mock_get_service, mock_get_service_template, mock_s3_upload ): @@ -123,6 +125,7 @@ def test_send_test_message_to_self( mocker, api_user_active, mock_login, + mock_get_service, mock_get_service_template, mock_s3_upload ): @@ -146,6 +149,7 @@ def test_download_example_csv( mocker, api_user_active, mock_login, + mock_get_service, mock_get_service_template ): @@ -166,6 +170,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mocker, api_user_active, mock_login, + mock_get_service, mock_get_service_template, mock_s3_upload ): @@ -205,6 +210,7 @@ def test_create_job_should_call_api( job_data, mock_create_job, mock_get_job, + mock_get_service, mock_get_service_template ): From 0de80bba9710bf3e5124185215246d167012da88 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Fri, 26 Feb 2016 13:07:35 +0000 Subject: [PATCH 11/20] [WIP] Invite user form now submits data to api. --- app/__init__.py | 3 +++ app/main/views/manage_users.py | 23 ++++++++++++++++++----- app/notify_client/invite_api_client.py | 23 +++++++++++++++++++++++ tests/app/main/views/test_manage_users.py | 14 ++++++++++---- tests/conftest.py | 14 +++++++++++++- 5 files changed, 67 insertions(+), 10 deletions(-) create mode 100644 app/notify_client/invite_api_client.py diff --git a/app/__init__.py b/app/__init__.py index cf4529634..fff052eb1 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -17,6 +17,7 @@ from app.notify_client.api_key_api_client import ApiKeyApiClient from app.notify_client.user_api_client import UserApiClient from app.notify_client.job_api_client import JobApiClient from app.notify_client.status_api_client import StatusApiClient +from app.notify_client.invite_api_client import InviteApiClient from app.its_dangerous_session import ItsdangerousSessionInterface from app.asset_fingerprinter import AssetFingerprinter from app.utils import validate_phone_number, InvalidPhoneError @@ -32,6 +33,7 @@ user_api_client = UserApiClient() api_key_api_client = ApiKeyApiClient() job_api_client = JobApiClient() status_api_client = StatusApiClient() +invite_api_client = InviteApiClient() asset_fingerprinter = AssetFingerprinter() @@ -49,6 +51,7 @@ def create_app(config_name, config_overrides=None): api_key_api_client.init_app(application) job_api_client.init_app(application) status_api_client.init_app(application) + invite_api_client.init_app(application) login_manager.init_app(application) login_manager.login_view = 'main.sign_in' diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 0a9a2d067..2f425f6af 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -7,13 +7,18 @@ from flask import ( flash ) -from flask_login import login_required, current_user +from flask_login import ( + login_required, + 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 InviteUserForm from app.main.dao.services_dao import get_service_by_id_or_404 from app import user_api_client +from app import invite_api_client fake_users = [ { @@ -44,10 +49,18 @@ def manage_users(service_id): def invite_user(service_id): form = InviteUserForm() - if form.validate_on_submit(): - flash('Invite sent to {}'.format(form.email_address.data), 'default_with_tick') - return redirect(url_for('.manage_users', service_id=service_id)) + email_address = form.email_address.data + try: + resp = invite_api_client.create_invite(current_user.id, service_id, email_address) + flash('Invite sent to {}'.format(resp['email_address']), 'default_with_tick') + return redirect(url_for('.manage_users', service_id=service_id)) + + except HTTPError as e: + if e.status_code == 404: + abort(404) + else: + raise e return render_template( 'views/invite-user.html', diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py new file mode 100644 index 000000000..e3a7e3ee3 --- /dev/null +++ b/app/notify_client/invite_api_client.py @@ -0,0 +1,23 @@ + +from notifications_python_client.base import BaseAPIClient + + +class InviteApiClient(BaseAPIClient): + def __init__(self, base_url=None, client_id=None, secret=None): + super(self.__class__, self).__init__(base_url=base_url or 'base_url', + client_id=client_id or 'client_id', + secret=secret or 'secret') + + def init_app(self, app): + self.base_url = app.config['API_HOST_NAME'] + self.client_id = app.config['ADMIN_CLIENT_USER_NAME'] + self.secret = app.config['ADMIN_CLIENT_SECRET'] + + def create_invite(self, invite_from_id, service_id, email_address): + data = { + 'service': str(service_id), + 'email_address': email_address, + 'from_user': invite_from_id + } + resp = self.post(url='/service/{}/invite'.format(service_id), data=data) + return resp['data'] diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index de146a148..bdd31ca70 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -70,19 +70,25 @@ def test_should_show_page_for_inviting_user( def test_invite_user( app_, + service_one, api_user_active, mock_login, - mock_get_service, - mock_get_users_by_service + mock_get_users_by_service, + mock_create_invite ): + from_user = api_user_active.id + service_id = service_one['id'] + email_address = 'test@example.gov.uk' + with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) response = client.post( - url_for('main.invite_user', service_id=55555), - data={'email_address': 'test@example.gov.uk'}, + url_for('main.invite_user', service_id=service_id), + data={'email_address': email_address}, follow_redirects=True ) assert response.status_code == 200 assert 'Invite sent to test@example.gov.uk' in response.get_data(as_text=True) + mock_create_invite.assert_called_with(from_user, service_id, email_address) diff --git a/tests/conftest.py b/tests/conftest.py index b9622bf9e..b0735c0f2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,4 @@ -import os +import uuid from datetime import date import pytest @@ -533,3 +533,15 @@ def mock_s3_upload(mocker): def _upload(upload_id, service_id, filedata, region): pass return mocker.patch('app.main.views.send.s3upload', side_effect=_upload) + + +@pytest.fixture(scope='function') +def mock_create_invite(mocker): + def _create_invite(from_user, service_id, email_address): + data = {'id': uuid.uuid4(), + 'from_user': from_user, + 'service': service_id, + 'email_address': email_address, + 'status': 'pending'} + return data + return mocker.patch('app.invite_api_client.create_invite', side_effect=_create_invite) From ce221fc40bcaab7b2d30d24f1e604093a9d661e1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Feb 2016 10:21:51 +0000 Subject: [PATCH 12/20] =?UTF-8?q?Drop=20=E2=80=98preview=20service=20name?= =?UTF-8?q?=E2=80=99=20page?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This page: - confused users in research - didn’t communicate what it was intended to (eg the generated email address, how your service name would appear in messages) This commit removes the page so that after typing in the service name the user is sent straight to the dashboard for their new service. --- app/main/views/add_service.py | 32 +++-------------- app/templates/views/add-from-address.html | 42 ----------------------- tests/app/main/views/test_add_service.py | 42 +---------------------- 3 files changed, 5 insertions(+), 111 deletions(-) delete mode 100644 app/templates/views/add-from-address.html diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index ad5d8f715..9cee69b8a 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -11,39 +11,15 @@ from app.main.forms import AddServiceForm @login_required def add_service(): form = AddServiceForm(services_dao.find_all_service_names) - services = services_dao.get_services(current_user.id) - if len(services['data']) == 0: - heading = 'Which service do you want to set up notifications for?' - else: - heading = 'Add a new service' + heading = 'Which service do you want to set up notifications for?' if form.validate_on_submit(): session['service_name'] = form.name.data - return redirect(url_for('main.add_from_address')) + user = users_dao.get_user_by_id(session['user_id']) + service_id = services_dao.insert_new_service(session['service_name'], user.id) + return redirect(url_for('main.service_dashboard', service_id=service_id)) else: return render_template( 'views/add-service.html', form=form, heading=heading ) - - -@main.route("/confirm-add-service", methods=['GET', 'POST']) -@login_required -def add_from_address(): - if request.method == 'POST': - user = users_dao.get_user_by_id(session['user_id']) - service_id = services_dao.insert_new_service(session['service_name'], user.id) - return redirect(url_for('main.service_dashboard', service_id=service_id)) - else: - return render_template( - 'views/add-from-address.html', - service_name=session['service_name'], - from_address="{}@notifications.service.gov.uk".format(_email_safe(session['service_name'])) - ) - - -def _email_safe(string): - return "".join([ - character.lower() if character.isalnum() or character == "." else "" - for character in re.sub("\s+", ".", string.strip()) - ]) diff --git a/app/templates/views/add-from-address.html b/app/templates/views/add-from-address.html deleted file mode 100644 index a67f3c6d3..000000000 --- a/app/templates/views/add-from-address.html +++ /dev/null @@ -1,42 +0,0 @@ -{% extends "withoutnav_template.html" %} -{% from "components/textbox.html" import textbox %} -{% from "components/page-footer.html" import page_footer %} -{% from "components/sms-message.html" import sms_message %} -{% from "components/email-message.html" import email_message %} - -{% block page_title %} - Preview your service name – GOV.UK Notify -{% endblock %} - -{% block maincolumn_content %} - -

    - Preview your service name -

    - -
    -
    - {{ sms_message( - "{}: we received your payment, thank you".format(service_name), - name="Text message", - recipient='Sent from 40604' - ) }} -
    -
    -
    -
    - {{ email_message( - subject="We received your payment, thank you", - body="Dear Alice Smith,\n\nThank you for…", - from_name=service_name, - from_address=from_address, - name="Email", - ) }} -
    -
    - -
    - {{page_footer('Looks good', back_link=url_for(".add_service"))}} -
    - -{% endblock %} diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index d8b344ea5..4661aa641 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -14,7 +14,7 @@ def test_get_should_render_add_service_template(app_, client.login(api_user_active) response = client.get(url_for('main.add_service')) assert response.status_code == 200 - assert 'Add a new service' in response.get_data(as_text=True) + assert 'Which service do you want to set up notifications for?' in response.get_data(as_text=True) def test_should_add_service_and_redirect_to_next_page(app_, @@ -31,48 +31,8 @@ def test_should_add_service_and_redirect_to_next_page(app_, url_for('main.add_service'), data={'name': 'testing the post'}) assert response.status_code == 302 - assert response.location == url_for('main.add_from_address', _external=True) - - -def test_should_confirm_add_service( - app_, - mock_login, - mock_get_services, - api_user_active, - mock_get_user, - mock_get_user_by_email -): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) - with client.session_transaction() as session: - session['service_name'] = 'Renew Your Pet Passport' - response = client.get(url_for('main.add_from_address')) - assert response.status_code == 200 - assert 'Preview your service name' in response.get_data(as_text=True) - assert 'Renew Your Pet Passport' in response.get_data(as_text=True) - assert 'renew.your.pet.passport@notifications.service.gov.uk' in response.get_data(as_text=True) - - -def test_should_add_service_after_confirmation( - app_, - mock_login, - mock_create_service, - mock_get_services, - api_user_active, - mock_get_user, - mock_get_user_by_email -): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(api_user_active) - with client.session_transaction() as session: - session['service_name'] = 'Renew Your Pet Passport' - response = client.post(url_for('main.add_from_address')) - assert response.status_code == 302 assert response.location == url_for('main.service_dashboard', service_id=101, _external=True) assert mock_create_service.called - assert mock_get_services.called def test_should_return_form_errors_when_service_name_is_empty(app_, From f129cc3db2eb14be04ae9482f783f6b3f843fd34 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Feb 2016 10:46:49 +0000 Subject: [PATCH 13/20] Reword the add service page Without the preview service name we probably have to go back to communicating a bit more on the add service page. This commit brings back the two bullet points about where the service name will appear, and tries to tie it into the nice words that Matt Sheret wrote for us. --- app/main/forms.py | 4 ++-- app/templates/views/add-service.html | 21 +++++++++++++++++---- tests/app/main/test_add_service_form.py | 2 +- tests/app/main/views/test_add_service.py | 4 ++-- 4 files changed, 22 insertions(+), 9 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index a2885fe8f..7bb000cc7 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -164,13 +164,13 @@ class AddServiceForm(Form): name = StringField( 'Service name', validators=[ - DataRequired(message='Service name can not be empty') + DataRequired(message='Service name can’t be empty') ] ) def validate_name(self, a): if a.data in self._names_func(): - raise ValidationError('Service name already exists') + raise ValidationError('This service name is already in use') class ServiceNameForm(Form): diff --git a/app/templates/views/add-service.html b/app/templates/views/add-service.html index f0b4a5ff7..ec03a91e9 100644 --- a/app/templates/views/add-service.html +++ b/app/templates/views/add-service.html @@ -16,16 +16,29 @@

    - Be specific. Remember that there might be other people in your - organisation using GOV.UK Notify. + Be specific to your service. Remember that there might be + other people in your organisation using GOV.UK Notify.

    +

    + Users will see this: +

    + +
      +
    • + at the start of every text message, eg ‘Vehicle tax: we received your + payment, thank you’ +
      • +
    • + as your email sender name +
      • +
    +
    {{ textbox(form.name, hint="You can change this later") }} - - {{ page_footer('Continue') }} + {{ page_footer('Add service') }}
    diff --git a/tests/app/main/test_add_service_form.py b/tests/app/main/test_add_service_form.py index a89170c1e..ca021b741 100644 --- a/tests/app/main/test_add_service_form.py +++ b/tests/app/main/test_add_service_form.py @@ -9,4 +9,4 @@ def test_form_should_have_errors_when_duplicate_service_is_added(app_): form = AddServiceForm(_get_form_names, formdata=MultiDict([('name', 'some service')])) form.validate() - assert {'name': ['Service name already exists']} == form.errors + assert {'name': ['This service name is already in use']} == form.errors diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 4661aa641..c5a4fc9e7 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -47,7 +47,7 @@ def test_should_return_form_errors_when_service_name_is_empty(app_, client.login(api_user_active) response = client.post(url_for('main.add_service'), data={}) assert response.status_code == 200 - assert 'Service name can not be empty' in response.get_data(as_text=True) + assert 'Service name can’t be empty' in response.get_data(as_text=True) def test_should_return_form_errors_with_duplicate_service_name(app_, @@ -62,5 +62,5 @@ def test_should_return_form_errors_with_duplicate_service_name(app_, response = client.post( url_for('main.add_service'), data={'name': 'service_one'}) assert response.status_code == 200 - assert 'Service name already exists' in response.get_data(as_text=True) + assert 'This service name is already in use' in response.get_data(as_text=True) assert mock_get_services.called From 1cbd37a9209a753a11bb930af2a7f745cd08762c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Feb 2016 13:19:46 +0000 Subject: [PATCH 14/20] Bring back SASS lint The problem with SASS lint that we found in https://github.com/alphagov/notifications-admin/pull/202 has been fixed (see https://github.com/sasstools/sass-lint/pull/550). This commit reverts #202, re-enabling linting of SASS files. --- gulpfile.babel.js | 9 ++++----- package.json | 1 + 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/gulpfile.babel.js b/gulpfile.babel.js index 6f2356c80..030c5eeb7 100644 --- a/gulpfile.babel.js +++ b/gulpfile.babel.js @@ -103,11 +103,10 @@ gulp.task('watchForChanges', function() { }); gulp.task('lint:sass', () => gulp -// Removed until gulp-sass-lint installs cleanly -// .src(paths.src + 'stylesheets/**/*.scss') -// .pipe(plugins.sassLint()) -// .pipe(plugins.sassLint.format(stylish)) -// .pipe(plugins.sassLint.failOnError()) + .src(paths.src + 'stylesheets/**/*.scss') + .pipe(plugins.sassLint()) + .pipe(plugins.sassLint.format(stylish)) + .pipe(plugins.sassLint.failOnError()) ); gulp.task('lint:js', () => gulp diff --git a/package.json b/package.json index a15a60448..6ed466634 100644 --- a/package.json +++ b/package.json @@ -38,6 +38,7 @@ "devDependencies": { "gulp-css-url-adjuster": "0.2.3", "gulp-jshint": "2.0.0", + "gulp-sass-lint": "1.1.1", "jshint": "2.9.1", "jshint-stylish": "2.1.0" } From 865ea3ca4f63f13da116e44ad78745f49a91b2bd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Feb 2016 15:00:32 +0000 Subject: [PATCH 15/20] Use correct API endpoint in docs This has confused people, should change it before the site visits next week. --- app/templates/views/documentation.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/documentation.html b/app/templates/views/documentation.html index bc9723c39..3512ae423 100644 --- a/app/templates/views/documentation.html +++ b/app/templates/views/documentation.html @@ -161,7 +161,7 @@

    API endpoint

    - https://www.notify.works/api/endpoint + https://api.notify.works

    From c76717942f96cd1b43284e45e4af170b1a6ec0c7 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Fri, 26 Feb 2016 15:33:17 +0000 Subject: [PATCH 16/20] Manage user pages now surfaces invited users fetched from api. --- app/main/views/manage_users.py | 21 +++++++++++++-------- app/notify_client/invite_api_client.py | 5 +++++ app/templates/views/manage-users.html | 2 +- tests/__init__.py | 9 +++++++++ tests/app/main/views/test_manage_users.py | 18 +++++++++++++----- tests/conftest.py | 17 ++++++++++++++++- 6 files changed, 57 insertions(+), 15 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 2f425f6af..843348bcf 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -34,14 +34,19 @@ fake_users = [ @main.route("/services//users") @login_required def manage_users(service_id): - users = user_api_client.get_users_for_service(service_id=service_id) - return render_template( - 'views/manage-users.html', - service_id=service_id, - users=users, - current_user=current_user, - invited_users=[] - ) + try: + users = user_api_client.get_users_for_service(service_id=service_id) + invited_users = invite_api_client.get_invites_for_service(service_id=service_id) + return render_template('views/manage-users.html', + service_id=service_id, + users=users, + current_user=current_user, + invited_users=invited_users) + except HTTPError as e: + if e.status_code == 404: + abort(404) + else: + raise e @main.route("/services//users/invite", methods=['GET', 'POST']) diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index e3a7e3ee3..7b30dcad6 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -21,3 +21,8 @@ class InviteApiClient(BaseAPIClient): } resp = self.post(url='/service/{}/invite'.format(service_id), data=data) return resp['data'] + + def get_invites_for_service(self, service_id): + endpoint = '/service/{}/invite'.format(service_id) + resp = self.get(endpoint) + return resp['data'] diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index d18341ab3..9fd509b65 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -41,7 +41,7 @@ Manage users – GOV.UK Notify invited_users, caption='Invited', **table_options ) %} {% call field() %} - {{ item.email_localpart }} + {{ item.email_address }} {% endcall %} {{ boolean_field(item.permission_send_messages) }} {{ boolean_field(item.permission_manage_service) }} diff --git a/tests/__init__.py b/tests/__init__.py index 850f1b53a..6327182c3 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -47,6 +47,15 @@ def api_key_json(id_, name, expiry_date=None): 'expiry_date': expiry_date } + +def invite_json(id, from_user, service_id, email_address): + return {'id': id, + 'from_user': from_user, + 'service': service_id, + 'email_address': email_address, + 'status': 'pending'} + + TEST_USER_EMAIL = 'test@user.gov.uk' diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index bdd31ca70..9a2e4a9b5 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1,13 +1,15 @@ -import json from flask import url_for +from bs4 import BeautifulSoup + def test_should_show_overview_page( app_, api_user_active, mock_login, mock_get_service, - mock_get_users_by_service + mock_get_users_by_service, + mock_get_invites_for_service ): with app_.test_request_context(): with app_.test_client() as client: @@ -38,7 +40,8 @@ def test_redirect_after_saving_user( api_user_active, mock_login, mock_get_service, - mock_get_users_by_service + mock_get_users_by_service, + mock_get_invites_for_service ): with app_.test_request_context(): with app_.test_client() as client: @@ -74,7 +77,8 @@ def test_invite_user( api_user_active, mock_login, mock_get_users_by_service, - mock_create_invite + mock_create_invite, + mock_get_invites_for_service ): from_user = api_user_active.id service_id = service_one['id'] @@ -90,5 +94,9 @@ def test_invite_user( ) assert response.status_code == 200 - assert 'Invite sent to test@example.gov.uk' in response.get_data(as_text=True) mock_create_invite.assert_called_with(from_user, service_id, email_address) + mock_get_invites_for_service.assert_called_with(service_id=service_id) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Manage team' + flash_banner = page.find('div', class_='banner-default-with-tick').string.strip() + assert flash_banner == 'Invite sent to test@example.gov.uk' diff --git a/tests/conftest.py b/tests/conftest.py index b0735c0f2..a4fce6a46 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -9,7 +9,8 @@ from . import ( TestClient, template_json, api_key_json, - job_json + job_json, + invite_json ) @@ -545,3 +546,17 @@ def mock_create_invite(mocker): 'status': 'pending'} return data return mocker.patch('app.invite_api_client.create_invite', side_effect=_create_invite) + + +@pytest.fixture(scope='function') +def mock_get_invites_for_service(mocker, service_one): + def _get_invites(service_id): + data = [] + from_user = service_one['users'][0] + service_id = service_one['id'] + for i in range(0, 5): + email_address = 'user_{}@testnotify.gov.uk'.format(i) + invite = invite_json(uuid.uuid4(), from_user, service_id, email_address) + data.append(invite) + return data + return mocker.patch('app.invite_api_client.get_invites_for_service', side_effect=_get_invites) From 0cce5db3d1a801eb3ad7826fcd7738a149bdbc4b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Feb 2016 16:22:07 +0000 Subject: [PATCH 17/20] Give users better guidance about CSV files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds some guidance on the ‘Add recipients’ page about what should be in the CSV file. The guidance is contextual to the template, and based on the problems that we saw users having yesterday. --- app/assets/stylesheets/_grids.scss | 4 ++++ app/templates/views/send.html | 28 +++++++++++++++++----------- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/app/assets/stylesheets/_grids.scss b/app/assets/stylesheets/_grids.scss index 7f8eb724f..5e8643173 100644 --- a/app/assets/stylesheets/_grids.scss +++ b/app/assets/stylesheets/_grids.scss @@ -15,3 +15,7 @@ margin-bottom: $gutter; clear: both; } + +.bottom-gutter-2-3 { + margin-bottom: $gutter * 2/3; +} diff --git a/app/templates/views/send.html b/app/templates/views/send.html index 25eda7364..7b0dcf027 100644 --- a/app/templates/views/send.html +++ b/app/templates/views/send.html @@ -30,19 +30,25 @@

    - {{ banner( - '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' - )}} +

    + Add recipients by uploading a .csv file with + {{ template.placeholders|length + 1 }} + {% if template.placeholders %} + columns: + {% else %} + column: + {% endif %} +

    +

    + to + {{ template.placeholders_as_markup|join(" ") }} +

    +

    + Download an example +

    - {{file_upload(form.file, button_text='Upload a CSV file')}} - -

    - Download an example CSV file -

    + {{file_upload(form.file, button_text='Upload your CSV file')}} {% endblock %} From 8c10c36f50cb46680aa3b476a01e667de9aed344 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Mon, 29 Feb 2016 11:03:35 +0000 Subject: [PATCH 18/20] Invite user form now posts permissions string to api with data to create invite. --- app/main/views/manage_users.py | 14 +++++++++++++- app/notify_client/invite_api_client.py | 5 +++-- app/templates/components/yes-no.html | 4 ++-- app/templates/views/invite-user.html | 1 + tests/app/main/views/test_manage_users.py | 8 ++++++-- tests/conftest.py | 5 +++-- 6 files changed, 28 insertions(+), 9 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 843348bcf..0b5f0e7f0 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -56,8 +56,9 @@ def invite_user(service_id): form = InviteUserForm() if form.validate_on_submit(): email_address = form.email_address.data + permissions = _get_permissions(request.form) try: - resp = invite_api_client.create_invite(current_user.id, service_id, email_address) + resp = invite_api_client.create_invite(current_user.id, service_id, email_address, permissions) flash('Invite sent to {}'.format(resp['email_address']), 'default_with_tick') return redirect(url_for('.manage_users', service_id=service_id)) @@ -113,3 +114,14 @@ def delete_user(service_id, user_id): service=get_service_by_id_or_404(service_id), service_id=service_id ) + + +def _get_permissions(form): + permissions = [] + if form.get('send_messages') and form['send_messages'] == 'yes': + permissions.append('send_messages') + if form.get('manage_service') and form['manage_service'] == 'yes': + permissions.append('manage_service') + if form.get('manage_api_keys') and form['manage_api_keys'] == 'yes': + permissions.append('manage_api_keys') + return ','.join(permissions) diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index 7b30dcad6..515422780 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -13,11 +13,12 @@ class InviteApiClient(BaseAPIClient): self.client_id = app.config['ADMIN_CLIENT_USER_NAME'] self.secret = app.config['ADMIN_CLIENT_SECRET'] - def create_invite(self, invite_from_id, service_id, email_address): + def create_invite(self, invite_from_id, service_id, email_address, permissions): data = { 'service': str(service_id), 'email_address': email_address, - 'from_user': invite_from_id + 'from_user': invite_from_id, + 'permissions': permissions } resp = self.post(url='/service/{}/invite'.format(service_id), data=data) return resp['data'] diff --git a/app/templates/components/yes-no.html b/app/templates/components/yes-no.html index 4432d6a9f..554a8e1fc 100644 --- a/app/templates/components/yes-no.html +++ b/app/templates/components/yes-no.html @@ -5,11 +5,11 @@
    diff --git a/app/templates/views/invite-user.html b/app/templates/views/invite-user.html index d1e6e5c7b..da40d42ff 100644 --- a/app/templates/views/invite-user.html +++ b/app/templates/views/invite-user.html @@ -28,6 +28,7 @@ Manage users – GOV.UK Notify Permissions + All team members can see message history {{ yes_no('send_messages', 'Send messages', user.permission_send_messages) }} {{ yes_no('manage_service', 'Manage service', user.permission_manage_service) }} {{ yes_no('manage_api_keys', 'Manage API keys', user.permission_manage_api_keys) }} diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 9a2e4a9b5..438e3060e 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -83,18 +83,22 @@ def test_invite_user( from_user = api_user_active.id service_id = service_one['id'] email_address = 'test@example.gov.uk' + permissions = 'send_messages,manage_service,manage_api_keys' with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) response = client.post( url_for('main.invite_user', service_id=service_id), - data={'email_address': email_address}, + data={'email_address': email_address, + 'send_messages': 'yes', + 'manage_service': 'yes', + 'manage_api_keys': 'yes'}, follow_redirects=True ) assert response.status_code == 200 - mock_create_invite.assert_called_with(from_user, service_id, email_address) + mock_create_invite.assert_called_with(from_user, service_id, email_address, permissions) mock_get_invites_for_service.assert_called_with(service_id=service_id) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.string.strip() == 'Manage team' diff --git a/tests/conftest.py b/tests/conftest.py index a4fce6a46..02bd09df4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -538,12 +538,13 @@ def mock_s3_upload(mocker): @pytest.fixture(scope='function') def mock_create_invite(mocker): - def _create_invite(from_user, service_id, email_address): + def _create_invite(from_user, service_id, email_address, permissions): data = {'id': uuid.uuid4(), 'from_user': from_user, 'service': service_id, 'email_address': email_address, - 'status': 'pending'} + 'status': 'pending', + 'permissions': permissions} return data return mocker.patch('app.invite_api_client.create_invite', side_effect=_create_invite) From 48943527ecc6badd849f156ab532783309f780b4 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Mon, 29 Feb 2016 14:57:07 +0000 Subject: [PATCH 19/20] API keys hooked up and working. All tests passing. --- app/__init__.py | 3 +++ app/main/views/api_keys.py | 4 ++++ app/main/views/dashboard.py | 2 ++ app/main/views/sms.py | 4 ---- app/notify_client/permission_api_client.py | 25 ++++++++++++++++++++++ app/notify_client/user_api_client.py | 18 ++++++---------- app/templates/main_nav.html | 6 ++++++ app/utils.py | 8 ++++--- tests/__init__.py | 5 +++-- tests/app/main/views/test_api_keys.py | 18 ++++++++++------ tests/conftest.py | 21 ++++++++++++++---- 11 files changed, 83 insertions(+), 31 deletions(-) create mode 100644 app/notify_client/permission_api_client.py diff --git a/app/__init__.py b/app/__init__.py index cf4529634..77cd41c86 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -17,6 +17,7 @@ from app.notify_client.api_key_api_client import ApiKeyApiClient from app.notify_client.user_api_client import UserApiClient from app.notify_client.job_api_client import JobApiClient from app.notify_client.status_api_client import StatusApiClient +from app.notify_client.permission_api_client import PermissionApiClient from app.its_dangerous_session import ItsdangerousSessionInterface from app.asset_fingerprinter import AssetFingerprinter from app.utils import validate_phone_number, InvalidPhoneError @@ -33,6 +34,7 @@ api_key_api_client = ApiKeyApiClient() job_api_client = JobApiClient() status_api_client = StatusApiClient() asset_fingerprinter = AssetFingerprinter() +permission_api_client = PermissionApiClient() def create_app(config_name, config_overrides=None): @@ -49,6 +51,7 @@ def create_app(config_name, config_overrides=None): api_key_api_client.init_app(application) job_api_client.init_app(application) status_api_client.init_app(application) + permission_api_client.init_app(application) login_manager.init_app(application) login_manager.login_view = 'main.sign_in' diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index 36959a86d..8053e2cf9 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -3,6 +3,7 @@ from flask_login import login_required from app.main import main from app.main.forms import CreateKeyForm from app import api_key_api_client +from app.utils import user_has_permissions @main.route("/services//documentation") @@ -13,6 +14,7 @@ def documentation(service_id): @main.route("/services//api-keys") @login_required +@user_has_permissions('manage_api_keys') def api_keys(service_id): return render_template( 'views/api-keys.html', @@ -23,6 +25,7 @@ def api_keys(service_id): @main.route("/services//api-keys/create", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_api_keys') def create_api_key(service_id): key_names = [ key['name'] for key in api_key_api_client.get_api_keys(service_id=service_id)['apiKeys'] @@ -41,6 +44,7 @@ def create_api_key(service_id): @main.route("/services//api-keys/revoke/", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_api_keys') def revoke_api_key(service_id, key_id): key_name = api_key_api_client.get_api_keys(service_id=service_id, key_id=key_id)['apiKeys'][0]['name'] if request.method == 'GET': diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index cb15fce90..f11722854 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -23,6 +23,8 @@ def service_dashboard(service_id): try: service = get_service_by_id(service_id) session['service_name'] = service['data']['name'] + print(service['data']['id']) + session['service_id'] = service['data']['id'] except HTTPError as e: if e.status_code == 404: abort(404) diff --git a/app/main/views/sms.py b/app/main/views/sms.py index 42340cbaa..2e3f69091 100644 --- a/app/main/views/sms.py +++ b/app/main/views/sms.py @@ -44,10 +44,6 @@ 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', templates=[ diff --git a/app/notify_client/permission_api_client.py b/app/notify_client/permission_api_client.py new file mode 100644 index 000000000..a112251af --- /dev/null +++ b/app/notify_client/permission_api_client.py @@ -0,0 +1,25 @@ +import uuid + +from notifications_python_client.base import BaseAPIClient + + +class PermissionApiClient(BaseAPIClient): + def __init__(self, base_url=None, client_id=None, secret=None): + super(self.__class__, self).__init__(base_url=base_url or 'base_url', + client_id=client_id or 'client_id', + secret=secret or 'secret') + + def init_app(self, app): + self.base_url = app.config['API_HOST_NAME'] + self.client_id = app.config['ADMIN_CLIENT_USER_NAME'] + self.secret = app.config['ADMIN_CLIENT_SECRET'] + + def delete_permission(self, permission_id): + return self.delete(url='/permission/{}'.format(permission_id))['data'] + + def create_permission(self, permission, user_id, service_id): + return self.post( + url='/permission', + data={'permission': permission, + 'user': user_id, + 'service': service_id})['data'] diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 693a321f7..dafd4f66b 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -89,7 +89,7 @@ class User(UserMixin): self._email_address = fields.get('email_address') self._mobile_number = fields.get('mobile_number') self._password_changed_at = fields.get('password_changed_at') - self._permissions = set(fields.get('permissions')) if fields.get('permission') is not None else set() + self._permissions = fields.get('permissions') self._failed_login_count = 0 self._state = fields.get('state') self.max_failed_login_count = max_failed_login_count @@ -154,18 +154,12 @@ class User(UserMixin): @permissions.setter def permissions(self, permissions): - if permissions is None: - permissions = set() - self._permissions = set(permissions) + raise AttributeError("Read only property") - def add_permissions(self, permissions): - self._permissions.update(permissions) - - def remove_permissions(self, permissions): - self._permissions -= permissions - - def has_permissions(self, permissions): - return self._permissions > set(permissions) + def has_permissions(self, service_id, permissions): + if service_id in self._permissions: + return set(self._permissions[service_id]) > set(permissions) + return False @property def failed_login_count(self): diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 4a379f3d8..2fb1c7186 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -2,16 +2,22 @@

    {{ session.get('service_name', 'Service') }}

    + {% if current_user.has_permissions(session.get('service_id', ''), ['send_messages']) %} + {% endif %} + {% if current_user.has_permissions(session.get('service_id', ''), ['manage_service']) %} + {% endif %} + {% if current_user.has_permissions(session.get('service_id', ''), ['manage_api_keys']) %} + {% endif %} diff --git a/app/utils.py b/app/utils.py index f814f4b81..ad89aea60 100644 --- a/app/utils.py +++ b/app/utils.py @@ -1,5 +1,5 @@ from functools import wraps -from flask import abort +from flask import (abort, session) class BrowsableItem(object): @@ -80,8 +80,10 @@ def user_has_permissions(*permissions): def wrap_func(*args, **kwargs): # We are making the assumption that the user is logged in. from flask_login import current_user - if set(permissions) > set(current_user.permissions): + service_id = session.get('service_id', '') + if current_user and current_user.has_permissions(service_id, permissions): + return func(*args, **kwargs) + else: abort(403) - return func(*args, **kwargs) return wrap_func return wrap diff --git a/tests/__init__.py b/tests/__init__.py index 850f1b53a..6170a7817 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -57,14 +57,15 @@ def create_test_user(state): return user -def create_test_api_user(state): +def create_test_api_user(state, permissions={}): from app.notify_client.user_api_client import User user_data = {'id': 1, 'name': 'Test User', 'password': 'somepassword', 'email_address': TEST_USER_EMAIL, 'mobile_number': '+441234123412', - 'state': state + 'state': state, + 'permissions': permissions } user = User(user_data) return user diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index aaba5f2b0..d22855fe6 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -21,7 +21,8 @@ def test_should_show_empty_api_keys_page(app_, mock_get_user, mock_get_user_by_email, mock_get_no_api_keys, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -39,7 +40,8 @@ def test_should_show_api_keys_page(app_, mock_get_user, mock_get_user_by_email, mock_get_api_keys, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -58,7 +60,8 @@ def test_should_show_name_api_key_page(app_, mock_get_user, mock_get_user_by_email, mock_get_api_keys, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -74,7 +77,8 @@ def test_should_render_show_api_key(app_, mock_get_user_by_email, mock_create_api_key, mock_get_api_keys, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -92,7 +96,8 @@ def test_should_show_confirm_revoke_api_key(app_, mock_get_user, mock_get_user_by_email, mock_get_api_keys, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -110,7 +115,8 @@ def test_should_redirect_after_revoking_api_key(app_, mock_get_user_by_email, mock_revoke_api_key, mock_get_api_keys, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) diff --git a/tests/conftest.py b/tests/conftest.py index 308ee42b7..9d6578fc0 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -195,7 +195,8 @@ def api_user_pending(): 'email_address': 'test@user.gov.uk', 'mobile_number': '+4412341234', 'state': 'pending', - 'failed_login_count': 0 + 'failed_login_count': 0, + 'permissions': {} } user = User(user_data) return user @@ -210,7 +211,8 @@ def api_user_active(): 'email_address': 'test@user.gov.uk', 'mobile_number': '+4412341234', 'state': 'active', - 'failed_login_count': 0 + 'failed_login_count': 0, + 'permissions': {} } user = User(user_data) return user @@ -225,7 +227,8 @@ def api_user_locked(): 'email_address': 'test@user.gov.uk', 'mobile_number': '+4412341234', 'state': 'active', - 'failed_login_count': 5 + 'failed_login_count': 5, + 'permissions': {} } user = User(user_data) return user @@ -240,7 +243,8 @@ def api_user_request_password_reset(): 'email_address': 'test@user.gov.uk', 'mobile_number': '+4412341234', 'state': 'request_password_reset', - 'failed_login_count': 5 + 'failed_login_count': 5, + 'permissions': {} } user = User(user_data) return user @@ -501,3 +505,12 @@ def mock_get_jobs(mocker): data.append(job_data) return {"data": data} return mocker.patch('app.job_api_client.get_job', side_effect=_get_jobs) + + +@pytest.fixture(scope='function') +def mock_has_permissions(mocker): + def _has_permission(service_id, permissions): + return True + return mocker.patch( + 'app.notify_client.user_api_client.User.has_permissions', + side_effect=_has_permission) From a8db812c5a9822f6ea72bf44134ed2219a2c5c74 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Mon, 29 Feb 2016 15:34:22 +0000 Subject: [PATCH 20/20] Remove print statement. --- app/main/views/dashboard.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index f11722854..9f7dc1044 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -23,7 +23,6 @@ def service_dashboard(service_id): try: service = get_service_by_id(service_id) session['service_name'] = service['data']['name'] - print(service['data']['id']) session['service_id'] = service['data']['id'] except HTTPError as e: if e.status_code == 404: