From ffe30c30706edb13fc06beb0286e117ca0a6d69a Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Mon, 29 Feb 2016 17:03:56 +0000 Subject: [PATCH 01/12] Permissions added for templates and send_messages pages. All tests passing. Fix up page heading. --- app/main/views/send.py | 23 ++++++++-- app/main/views/service_settings.py | 9 ++++ app/main/views/templates.py | 4 ++ app/notify_client/user_api_client.py | 4 +- app/templates/main_nav.html | 6 +++ app/templates/views/choose-template.html | 8 ++++ app/utils.py | 4 +- tests/app/main/views/test_send.py | 24 ++++++---- tests/app/main/views/test_service_settings.py | 45 ++++++++++++------- tests/app/main/views/test_templates.py | 12 +++-- tests/conftest.py | 2 +- 11 files changed, 106 insertions(+), 35 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 856403105..a626dfded 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -26,11 +26,17 @@ from app.main.uploader import ( 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_recipient, InvalidPhoneError, InvalidEmailError +from app.utils import ( + validate_recipient, InvalidPhoneError, InvalidEmailError, user_has_permissions) page_headings = { - 'email': 'Send emails', - 'sms': 'Send text messages' + 'manage_service': { + 'email': 'Send emails', + 'sms': 'Send text messages'}, + 'manage_templates': { + 'email': 'Manage templates', + 'sms': 'Manage templates' + } } @@ -42,6 +48,8 @@ def letters_stub(service_id): @main.route("/services//send/", methods=['GET']) +@login_required +@user_has_permissions('send_messages', 'manage_templates', or_=True) def choose_template(service_id, template_type): service = services_dao.get_service_by_id_or_404(service_id) @@ -55,6 +63,9 @@ def choose_template(service_id, template_type): abort(404) else: raise e + # TODO fix up how page_heading is loaded. + page_heading = page_headings['manage_service'][template_type] if current_user.has_permissions(session.get('service_id', ''), 'manage_service') else \ + page_headings['manage_templates'][template_type] return render_template( 'views/choose-template.html', templates=[ @@ -65,7 +76,7 @@ def choose_template(service_id, template_type): if template['template_type'] == template_type ], template_type=template_type, - page_heading=page_headings[template_type], + page_heading=page_heading, service=service, has_jobs=len(jobs), service_id=service_id @@ -74,6 +85,7 @@ def choose_template(service_id, template_type): @main.route("/services//send/", methods=['GET', 'POST']) @login_required +@user_has_permissions('send_messages') def send_messages(service_id, template_id): form = CsvUploadForm() @@ -110,6 +122,7 @@ def send_messages(service_id, template_id): @main.route("/services//send/.csv", methods=['GET']) @login_required +@user_has_permissions('send_messages', 'manage_templates', or_=True) def get_example_csv(service_id, template_id): template = templates_dao.get_service_template_or_404(service_id, template_id)['data'] placeholders = list(Template(template).placeholders) @@ -127,6 +140,7 @@ def get_example_csv(service_id, template_id): @main.route("/services//send//to-self", methods=['GET']) @login_required +@user_has_permissions('send_messages') 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) @@ -150,6 +164,7 @@ def send_message_to_self(service_id, template_id): @main.route("/services//check/", methods=['GET', 'POST']) @login_required +@user_has_permissions('send_messages') def check_messages(service_id, upload_id): upload_data = session['upload_data'] diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 12b68f307..df2b1f2f7 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -3,6 +3,7 @@ from flask import ( from flask_login import (login_required, current_user) from app.main import main +from app.utils import user_has_permissions from app.main.dao.services_dao import ( get_service_by_id, delete_service, update_service) from app.main.dao.users_dao import verify_password @@ -12,6 +13,7 @@ from notifications_python_client.errors import HTTPError @main.route("/services//service-settings") @login_required +@user_has_permissions('manage_service') def service_settings(service_id): try: service = get_service_by_id(service_id)['data'] @@ -29,6 +31,7 @@ def service_settings(service_id): @main.route("/services//service-settings/name", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_name_change(service_id): try: service = get_service_by_id(service_id)['data'] @@ -53,6 +56,7 @@ def service_name_change(service_id): @main.route("/services//service-settings/name/confirm", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_name_change_confirm(service_id): try: service = get_service_by_id(service_id)['data'] @@ -82,6 +86,7 @@ def service_name_change_confirm(service_id): @main.route("/services//service-settings/request-to-go-live", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_request_to_go_live(service_id): try: service = get_service_by_id(service_id)['data'] @@ -104,6 +109,7 @@ def service_request_to_go_live(service_id): @main.route("/services//service-settings/status", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_status_change(service_id): try: service = get_service_by_id(service_id)['data'] @@ -125,6 +131,7 @@ def service_status_change(service_id): @main.route("/services//service-settings/status/confirm", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_status_change_confirm(service_id): try: service = get_service_by_id(service_id)['data'] @@ -153,6 +160,7 @@ def service_status_change_confirm(service_id): @main.route("/services//service-settings/delete", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_delete(service_id): try: service = get_service_by_id(service_id)['data'] @@ -174,6 +182,7 @@ def service_delete(service_id): @main.route("/services//service-settings/delete/confirm", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_service') def service_delete_confirm(service_id): try: service = get_service_by_id(service_id)['data'] diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 15f72b113..e178ea13e 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -5,6 +5,7 @@ from notifications_python_client.errors import HTTPError from utils.template import Template from app.main import main +from app.utils import user_has_permissions from app.main.forms import SMSTemplateForm, EmailTemplateForm from app import job_api_client from app.main.dao.services_dao import get_service_by_id_or_404 @@ -20,6 +21,7 @@ form_objects = { @main.route("/services//templates/add-", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_templates') def add_service_template(service_id, template_type): service = sdao.get_service_by_id_or_404(service_id) @@ -51,6 +53,7 @@ def add_service_template(service_id, template_type): @main.route("/services//templates/", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_templates') 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'] @@ -78,6 +81,7 @@ def edit_service_template(service_id, template_id): @main.route("/services//templates//delete", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_templates') def delete_service_template(service_id, template_id): template = tdao.get_service_template_or_404(service_id, template_id)['data'] diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index e0d1057d1..46bdbb2d9 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -167,8 +167,10 @@ class User(UserMixin): def permissions(self, permissions): raise AttributeError("Read only property") - def has_permissions(self, service_id, permissions): + def has_permissions(self, service_id, permissions, or_=False): if service_id in self._permissions: + if or_: + return any([x in self._permissions[service_id] for x in permissions]) return set(self._permissions[service_id]) > set(permissions) return False diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 216bec23c..cfc6cd0d1 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -8,6 +8,12 @@
  • Send emails
  • Send letters
    • + {% elif current_user.has_permissions(session.get('service_id', ''), ['manage_templates']) %} + {% endif %} {% if current_user.has_permissions(session.get('service_id', ''), ['manage_service']) %}
      diff --git a/app/templates/views/choose-template.html b/app/templates/views/choose-template.html index 7fb87b827..b2d9545f5 100644 --- a/app/templates/views/choose-template.html +++ b/app/templates/views/choose-template.html @@ -16,6 +16,7 @@ {% if templates %} {% if not has_jobs %} + {% if current_user.has_permissions(session.get('service_id', ''), ['manage_service']) %} {{ banner( """ Send yourself a test message @@ -23,6 +24,7 @@ subhead='Next step', type="tip" )}} + {% endif %} {% endif %}
      {% for template in templates %} @@ -39,9 +41,13 @@
      + {% if current_user.has_permissions(session.get('service_id', ''), ['manage_service']) %} Add recipients Send yourself a test + {% endif %} + {% if current_user.has_permissions(session.get('service_id', ''), ['manage_templates']) %} Edit template + {% endif %}
      {% endfor %} @@ -49,7 +55,9 @@ {% endif %}

      + {% if current_user.has_permissions(session.get('service_id', ''), ['manage_templates']) %} Add a new template + {% endif %}

      diff --git a/app/utils.py b/app/utils.py index 176ff26cb..280b052e1 100644 --- a/app/utils.py +++ b/app/utils.py @@ -94,14 +94,14 @@ def validate_recipient(recipient, template_type): }[template_type](recipient) -def user_has_permissions(*permissions): +def user_has_permissions(*permissions, or_=False): def wrap(func): @wraps(func) def wrap_func(*args, **kwargs): # We are making the assumption that the user is logged in. from flask_login import current_user service_id = session.get('service_id', '') - if current_user and current_user.has_permissions(service_id, permissions): + if current_user and current_user.has_permissions(service_id, permissions, or_=or_): return func(*args, **kwargs) else: abort(403) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index e638a5481..28116626c 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -17,7 +17,8 @@ def test_choose_template( mock_get_service, mock_check_verify_code, mock_get_service_templates, - mock_get_jobs + mock_get_jobs, + mock_has_permissions ): with app_.test_request_context(): with app_.test_client() as client: @@ -40,7 +41,8 @@ def test_upload_empty_csvfile_returns_to_upload_page( mock_get_service, mock_get_service_templates, mock_check_verify_code, - mock_get_service_template + mock_get_service_template, + mock_has_permissions ): with app_.test_request_context(): with app_.test_client() as client: @@ -64,7 +66,8 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors( mock_login, mock_get_service, mock_get_service_template, - mock_s3_upload + mock_s3_upload, + mock_has_permissions ): contents = 'to,name\n+44 123,test1\n+44 456,test2' @@ -95,7 +98,8 @@ def test_upload_csvfile_removes_empty_lines_and_trailing_commas( mock_login, mock_get_service, mock_get_service_template, - mock_s3_upload + mock_s3_upload, + mock_has_permissions ): contents = 'to,name,,,\n++44 7700 900981,test1,,,\n+44 7700 900981,test2,,,\n ,,, \n ,,, \t \t \n' @@ -127,7 +131,8 @@ def test_send_test_message_to_self( mock_login, mock_get_service, mock_get_service_template, - mock_s3_upload + mock_s3_upload, + mock_has_permissions ): expected_data = {'data': ['to', '+4412341234'], 'file_name': 'Test run'} @@ -150,7 +155,8 @@ def test_download_example_csv( api_user_active, mock_login, mock_get_service, - mock_get_service_template + mock_get_service_template, + mock_has_permissions ): with app_.test_request_context(): @@ -172,7 +178,8 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mock_login, mock_get_service, mock_get_service_template, - mock_s3_upload + mock_s3_upload, + mock_has_permissions ): 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 @@ -211,7 +218,8 @@ def test_create_job_should_call_api( mock_create_job, mock_get_job, mock_get_service, - mock_get_service_template + mock_get_service_template, + mock_has_permissions ): service_id = service_one['id'] diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 045cc4c86..174935f2a 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -6,7 +6,8 @@ def test_should_show_overview(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -25,7 +26,8 @@ def test_should_show_service_name(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -44,7 +46,8 @@ def test_should_redirect_after_change_service_name(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -64,7 +67,8 @@ def test_should_show_service_name_confirmation(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -85,7 +89,8 @@ def test_should_redirect_after_service_name_confirmation(app_, mock_get_user, mock_get_user_by_email, mock_login, - mock_verify_password): + mock_verify_password, + 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_show_request_to_go_live(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -130,7 +136,8 @@ def test_should_redirect_after_request_to_go_live(app_, mock_update_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -151,7 +158,8 @@ def test_should_show_status_page(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -170,7 +178,8 @@ def test_should_show_redirect_after_status_change(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -190,7 +199,8 @@ def test_should_show_status_confirmation(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -211,7 +221,8 @@ def test_should_redirect_after_status_confirmation(app_, mock_get_user, mock_get_user_by_email, mock_login, - mock_verify_password): + mock_verify_password, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -232,7 +243,8 @@ def test_should_show_delete_page(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -250,7 +262,8 @@ def test_should_show_redirect_after_deleting_service(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -269,7 +282,8 @@ def test_should_show_delete_confirmation(app_, mock_get_service, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -289,7 +303,8 @@ def test_should_redirect_delete_confirmation(app_, mock_get_user, mock_get_user_by_email, mock_login, - mock_verify_password): + mock_verify_password, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 1ec43d502..cff04bde1 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -9,7 +9,8 @@ def test_should_show_page_for_one_templates(app_, mock_get_service_template, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -33,7 +34,8 @@ def test_should_redirect_when_saving_a_template(app_, mock_update_service_template, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -65,7 +67,8 @@ def test_should_show_delete_template_page(app_, mock_get_service_template, mock_get_user, mock_get_user_by_email, - mock_login): + mock_login, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -91,7 +94,8 @@ def test_should_redirect_when_deleting_a_template(app_, mock_delete_service_template, mock_get_user, mock_get_user_by_email, - 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 a8614c429..3afba0d1b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -519,7 +519,7 @@ def mock_get_jobs(mocker): @pytest.fixture(scope='function') def mock_has_permissions(mocker): - def _has_permission(service_id, permissions): + def _has_permission(service_id, permissions, or_=False): return True return mocker.patch( 'app.notify_client.user_api_client.User.has_permissions', From efaf826e867235d1cc45d8d3a44257702ec86f11 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Tue, 1 Mar 2016 15:53:58 +0000 Subject: [PATCH 02/12] Merge with master. --- app/main/views/send.py | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index a626dfded..c22d1fb08 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -29,17 +29,26 @@ from app import job_api_client from app.utils import ( validate_recipient, InvalidPhoneError, InvalidEmailError, user_has_permissions) -page_headings = { - 'manage_service': { - 'email': 'Send emails', - 'sms': 'Send text messages'}, - 'manage_templates': { - 'email': 'Manage templates', - 'sms': 'Manage templates' - } + +manage_service_page_headings = { + 'email': 'Send emails', + 'sms': 'Send text messages' } +manage_templates_page_headings = { + 'email': 'Manage templates', + 'sms': 'Manage templates' +} + + +def get_page_headings(template_type): + if current_user.has_permissions(session.get('service_id', ''), 'manage_service'): + return manage_service_page_headings[template_type] + else: + return manage_templates_page_headings[template_type] + + @main.route("/services//send/letters", methods=['GET']) def letters_stub(service_id): return render_template( @@ -63,9 +72,6 @@ def choose_template(service_id, template_type): abort(404) else: raise e - # TODO fix up how page_heading is loaded. - page_heading = page_headings['manage_service'][template_type] if current_user.has_permissions(session.get('service_id', ''), 'manage_service') else \ - page_headings['manage_templates'][template_type] return render_template( 'views/choose-template.html', templates=[ @@ -76,7 +82,7 @@ def choose_template(service_id, template_type): if template['template_type'] == template_type ], template_type=template_type, - page_heading=page_heading, + page_heading=get_page_headings(template_type), service=service, has_jobs=len(jobs), service_id=service_id @@ -188,7 +194,7 @@ def check_messages(service_id, upload_id): 'views/check.html', upload_result=upload_result, template=template, - page_heading=page_headings[template.template_type], + page_heading=get_page_headings(template.template_type), column_headers=['to'] + list(template.placeholders_as_markup), original_file_name=upload_data.get('original_file_name'), service_id=service_id, From 5b073341a09d8e5bffb48475e1c9e4df6252b393 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 3 Mar 2016 07:41:53 +0000 Subject: [PATCH 03/12] Add frontend install and build to bootstrap script MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now you don’t need to remember what the two commands are and when you need to run them, you can just run the bootstrap script instead. Makes sense to have them here since the `pip install` is also in here. --- README.md | 43 +++++++++++++++++-------------------------- scripts/bootstrap.sh | 2 ++ 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 6e0f3b741..23dd3de7a 100644 --- a/README.md +++ b/README.md @@ -4,52 +4,49 @@ # notifications-admin -Application to handle the admin functions of the notifications application. +GOV.UK Notify admin application. ## Features of this application - - Register users - - Register services - - Download CSV for an email or SMS batch + - Register and manage users + - Create and manage services + - Send batch emails and SMS by uploading a CSV - Show history of notifications - - Reports ## First-time setup -You need [Node](http://nodejs.org/) which will also get you [NPM](npmjs.org), -Node's package management tool. +Languages needed +- Python 3 +- [Node](http://nodejs.org/) 5.0.0 or greater ```shell brew install node ``` -n is a tool for managing different versions of node. The following installs n -and uses the latest version of node. +[NPM](npmjs.org) is Node's package management tool. `n` is a tool for managing +different versions of Node. The following installs `n` and uses the latest +version of Node. ```shell npm install -g n n latest npm rebuild node-sass ``` -The frontend dependencies are managed using NPM and Bower. To install or update -*all the things*, run -```shell - npm install - npm run build -``` - The app runs within a virtual environment. To [install virtualenv](https://virtualenv.readthedocs.org/en/latest/installation.html), run ```shell [sudo] pip install virtualenv ``` -To make a virtualenv for this app, run +Make a virtual environment for this app: ```shell mkvirtualenv -p /usr/local/bin/python3 notifications-admin - pip install -r requirements.txt +``` + +Install dependencies and build the frontend assets: +```shell ./scripts/bootstrap.sh ``` -## Building the frontend +## Rebuilding the frontend assets If you want the front end assets to re-compile on changes, leave this running in a separate terminal from the app @@ -64,10 +61,4 @@ in a separate terminal from the app ./scripts/run_app.sh ``` -Then visit [localhost:6012](localhost:6012) - -## Domain model - -All the domain models are defined in the -[models.py](https://github.com/alphagov/notifications-admin/blob/master/app/models.py) -file. +Then visit [localhost:6012](http://localhost:6012) diff --git a/scripts/bootstrap.sh b/scripts/bootstrap.sh index cd3ffb715..82274e091 100755 --- a/scripts/bootstrap.sh +++ b/scripts/bootstrap.sh @@ -27,3 +27,5 @@ fi # Install Python development dependencies pip3 install -r requirements_for_test.txt + +npm install && npm run build From 1b59e5c7f11de3d5804b9fc514786ea86106a411 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Thu, 3 Mar 2016 09:02:56 +0000 Subject: [PATCH 04/12] Review comments fixed. All tests passing. --- app/main/views/send.py | 2 +- app/notify_client/models.py | 5 +- app/templates/main_nav.html | 12 ++--- app/templates/views/choose-template.html | 8 +-- app/utils.py | 4 +- tests/app/main/test_utils.py | 65 ++++++++++++++++++------ tests/conftest.py | 11 +++- 7 files changed, 76 insertions(+), 31 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 37415b54b..1c3e9f206 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -44,7 +44,7 @@ manage_templates_page_headings = { def get_page_headings(template_type): - if current_user.has_permissions(session.get('service_id', ''), 'manage_service'): + if current_user.has_permissions(['manage_service']): return manage_service_page_headings[template_type] else: return manage_templates_page_headings[template_type] diff --git a/app/notify_client/models.py b/app/notify_client/models.py index 3cf6497b6..b881e8390 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -1,4 +1,5 @@ from flask.ext.login import (UserMixin, login_fresh) +from flask import session class User(UserMixin): @@ -81,7 +82,9 @@ class User(UserMixin): def permissions(self, permissions): raise AttributeError("Read only property") - def has_permissions(self, service_id, permissions, or_=False): + def has_permissions(self, permissions, service_id=None, or_=False): + if service_id is None: + service_id = session.get('service_id', '') if service_id in self._permissions: if or_: return any([x in self._permissions[service_id] for x in permissions]) diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index cfc6cd0d1..96a360727 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -2,26 +2,26 @@

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

      - {% if current_user.has_permissions(session.get('service_id', ''), ['send_messages']) %} + {% if current_user.has_permissions(['send_messages']) %} - {% elif current_user.has_permissions(session.get('service_id', ''), ['manage_templates']) %} + {% elif current_user.has_permissions(['manage_templates']) %} {% endif %} - {% if current_user.has_permissions(session.get('service_id', ''), ['manage_service']) %} + {% if current_user.has_permissions(['manage_service']) %} {% endif %} - {% if current_user.has_permissions(session.get('service_id', ''), ['manage_api_keys']) %} + {% if current_user.has_permissions(['manage_api_keys']) %}
      • API keys
      • Developer documentation
        • diff --git a/app/templates/views/choose-template.html b/app/templates/views/choose-template.html index b2d9545f5..bf6f08f12 100644 --- a/app/templates/views/choose-template.html +++ b/app/templates/views/choose-template.html @@ -16,7 +16,7 @@ {% if templates %} {% if not has_jobs %} - {% if current_user.has_permissions(session.get('service_id', ''), ['manage_service']) %} + {% if current_user.has_permissions(['manage_service']) %} {{ banner( """ Send yourself a test message @@ -41,11 +41,11 @@
        - {% if current_user.has_permissions(session.get('service_id', ''), ['manage_service']) %} + {% if current_user.has_permissions(['manage_service']) %} Add recipients Send yourself a test {% endif %} - {% if current_user.has_permissions(session.get('service_id', ''), ['manage_templates']) %} + {% if current_user.has_permissions(['manage_templates']) %} Edit template {% endif %}
        @@ -55,7 +55,7 @@ {% endif %}

        - {% if current_user.has_permissions(session.get('service_id', ''), ['manage_templates']) %} + {% if current_user.has_permissions(['manage_templates']) %} Add a new template {% endif %}

        diff --git a/app/utils.py b/app/utils.py index 965939028..11eb18876 100644 --- a/app/utils.py +++ b/app/utils.py @@ -100,10 +100,8 @@ def user_has_permissions(*permissions, or_=False): def wrap(func): @wraps(func) def wrap_func(*args, **kwargs): - # We are making the assumption that the user is logged in. from flask_login import current_user - service_id = session.get('service_id', '') - if current_user and current_user.has_permissions(service_id, permissions, or_=or_): + if current_user and current_user.has_permissions(permissions, or_=or_): return func(*args, **kwargs) else: abort(403) diff --git a/tests/app/main/test_utils.py b/tests/app/main/test_utils.py index 67262c360..95b435f6e 100644 --- a/tests/app/main/test_utils.py +++ b/tests/app/main/test_utils.py @@ -6,18 +6,53 @@ from app.main.views.index import index from werkzeug.exceptions import Forbidden -# def test_user_has_permissions(app_, -# api_user_active, -# mock_get_user, -# mock_get_user_by_email, -# mock_login): -# with app_.test_request_context(): -# with app_.test_client() as client: -# client.login(api_user_active) -# decorator = user_has_permissions('something') -# decorated_index = decorator(index) -# try: -# response = decorated_index() -# pytest.fail("Failed to throw a forbidden exception") -# except Forbidden: -# pass +def test_user_has_permissions_on_endpoint_fail(app_, + api_user_active, + mock_login, + mock_get_user_with_permissions): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + decorator = user_has_permissions('something') + decorated_index = decorator(index) + try: + response = decorated_index() + pytest.fail("Failed to throw a forbidden exception") + except Forbidden: + pass + + +def test_user_has_permissions_success(app_, + api_user_active, + mock_login, + mock_get_user_with_permissions): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + decorator = user_has_permissions('manage_users') + decorated_index = decorator(index) + response = decorated_index() + + +def test_user_has_permissions_or(app_, + api_user_active, + mock_login, + mock_get_user_with_permissions): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + decorator = user_has_permissions('something', 'manage_users', or_=True) + decorated_index = decorator(index) + response = decorated_index() + + +def test_user_has_permissions_multiple(app_, + api_user_active, + mock_login, + mock_get_user_with_permissions): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + decorator = user_has_permissions('manage_templates', 'manage_users') + decorated_index = decorator(index) + response = decorated_index() diff --git a/tests/conftest.py b/tests/conftest.py index 81c159cf2..a3d4f54aa 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -305,6 +305,15 @@ def mock_get_user_by_email(mocker, api_user_active): return mocker.patch('app.user_api_client.get_user_by_email', side_effect=_get_user) +@pytest.fixture(scope='function') +def mock_get_user_with_permissions(mocker, api_user_active): + def _get_user(id): + api_user_active._permissions[''] = ['manage_users', 'manage_templates', 'manage_settings'] + return api_user_active + return mocker.patch( + 'app.user_api_client.get_user', side_effect=_get_user) + + @pytest.fixture(scope='function') def mock_dont_get_user_by_email(mocker): @@ -523,7 +532,7 @@ def mock_get_jobs(mocker): @pytest.fixture(scope='function') def mock_has_permissions(mocker): - def _has_permission(service_id, permissions, or_=False): + def _has_permission(permissions, service_id=None, or_=False): return True return mocker.patch( 'app.notify_client.user_api_client.User.has_permissions', From e3fce349e1f822bf3a810da52332e6a8e02715c8 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Thu, 3 Mar 2016 09:30:30 +0000 Subject: [PATCH 05/12] Fix tests with master. --- tests/app/main/views/test_send.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index e1e4762e9..9c7e2a30f 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -156,7 +156,8 @@ def test_send_test_message_to_self( mock_login, mock_get_service, mock_get_service_email_template, - mock_s3_upload + mock_s3_upload, + mock_has_permissions ): expected_data = {'data': ['email address', 'test@user.gov.uk'], 'file_name': 'Test run'} From 1f2fe2a1e4b1b0a8baca73d7b8db7ec9355cc469 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Thu, 3 Mar 2016 10:24:49 +0000 Subject: [PATCH 06/12] Fixed up client for permission setting. All tests passing. --- app/__init__.py | 3 --- app/notify_client/permission_api_client.py | 25 ---------------------- app/notify_client/user_api_client.py | 5 +++++ 3 files changed, 5 insertions(+), 28 deletions(-) delete mode 100644 app/notify_client/permission_api_client.py diff --git a/app/__init__.py b/app/__init__.py index b9e070318..f7f98a97b 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -15,7 +15,6 @@ 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.notify_client.invite_api_client import InviteApiClient from app.its_dangerous_session import ItsdangerousSessionInterface from app.asset_fingerprinter import AssetFingerprinter @@ -34,7 +33,6 @@ job_api_client = JobApiClient() status_api_client = StatusApiClient() invite_api_client = InviteApiClient() asset_fingerprinter = AssetFingerprinter() -permission_api_client = PermissionApiClient() def create_app(config_name, config_overrides=None): @@ -51,7 +49,6 @@ 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) invite_api_client.init_app(application) login_manager.init_app(application) diff --git a/app/notify_client/permission_api_client.py b/app/notify_client/permission_api_client.py deleted file mode 100644 index a112251af..000000000 --- a/app/notify_client/permission_api_client.py +++ /dev/null @@ -1,25 +0,0 @@ -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 6b3450503..d825f079f 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -98,3 +98,8 @@ class UserApiClient(BaseAPIClient): endpoint = '/service/{}/users/{}'.format(service_id, user_id) resp = self.post(endpoint, data={}) return User(resp['data'], max_failed_login_count=self.max_failed_login_count) + + def set_user_permissions(self, user_id, service_id, permissions): + data = [{'permission': x} for x in permissions] + endpoint = '/user/{}/service/{}/permission'.format(user_id, service_id) + resp = self.post(endpoint, data=data) From 542f097b6d738e76fb943fca67ca7296c455f593 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 2 Mar 2016 13:17:24 +0000 Subject: [PATCH 07/12] Make the template preview work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The template was displaying raw, eg ‘Hello ((name))’ This commit changes it to use the `.formatted_as_markup` property so the template is rendered with: - the placeholders as blue lozenges - the service name prefixing the message --- app/templates/views/job.html | 2 +- tests/app/main/views/test_jobs.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/templates/views/job.html b/app/templates/views/job.html index 416da439e..b10e5d89f 100644 --- a/app/templates/views/job.html +++ b/app/templates/views/job.html @@ -19,7 +19,7 @@
        {{ sms_message( - template, + template.formatted_as_markup, )}}
        diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index c3147555f..6e5574061 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -41,4 +41,6 @@ def test_should_show_page_for_one_job(app_, response = client.get(url_for('main.view_job', service_id=service_id, job_id=job_id)) assert response.status_code == 200 - assert file_name in response.get_data(as_text=True) + content = response.get_data(as_text=True) + assert "Test Service: Your vehicle tax is about to expire" in content + assert file_name in content From 2f76ef27ce09f555e26c3b0a01bf93c8f3376bf0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 2 Mar 2016 13:52:05 +0000 Subject: [PATCH 08/12] Add API client for notifications This commit adds an API client for the notifications endpoints added in: https://github.com/alphagov/notifications-api/pull/113 --- app/notify_client/notification_api_client.py | 37 +++++++++++++++++ .../notify_client/test_notification_client.py | 41 +++++++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 app/notify_client/notification_api_client.py create mode 100644 tests/app/main/notify_client/test_notification_client.py diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py new file mode 100644 index 000000000..fe253669a --- /dev/null +++ b/app/notify_client/notification_api_client.py @@ -0,0 +1,37 @@ +from notifications_python_client.base import BaseAPIClient + + +class NotificationApiClient(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 get_all_notifications(self, page=None): + params = {} + if page is not None: + params['page'] = page + return self.get( + url='/notifications', + params=params + ) + + def get_notifications_for_service(self, service_id, job_id=None, page=None): + params = {} + if page is not None: + params['page'] = page + if job_id: + return self.get( + url='/service/{}/job/{}/notifications'.format(service_id, job_id), + params=params + ) + else: + return self.get( + url='/service/{}/notifications'.format(service_id), + params=params + ) diff --git a/tests/app/main/notify_client/test_notification_client.py b/tests/app/main/notify_client/test_notification_client.py new file mode 100644 index 000000000..08db26780 --- /dev/null +++ b/tests/app/main/notify_client/test_notification_client.py @@ -0,0 +1,41 @@ +import pytest +from app.notify_client.notification_api_client import NotificationApiClient + + +def test_client_gets_notifications(mocker): + + mock_get = mocker.patch('app.notify_client.notification_api_client.NotificationApiClient.get') + NotificationApiClient().get_all_notifications() + mock_get.assert_called_once_with(url='/notifications', params={}) + + +def test_client_gets_notifications_with_page(mocker): + + mock_get = mocker.patch('app.notify_client.notification_api_client.NotificationApiClient.get') + NotificationApiClient().get_all_notifications(page=99) + mock_get.assert_called_once_with(url='/notifications', params={'page': 99}) + + +@pytest.mark.parametrize("arguments,expected_call", [ + ( + {}, + {'url': '/service/abcd1234/notifications', 'params': {}} + ), + ( + {'page': 99}, + {'url': '/service/abcd1234/notifications', 'params': {'page': 99}} + ), + ( + {'job_id': 'efgh5678'}, + {'url': '/service/abcd1234/job/efgh5678/notifications', 'params': {}} + ), + ( + {'job_id': 'efgh5678', 'page': 48}, + {'url': '/service/abcd1234/job/efgh5678/notifications', 'params': {'page': 48}} + ) +]) +def test_client_gets_notifications_for_service_and_job_by_page(mocker, arguments, expected_call): + + mock_get = mocker.patch('app.notify_client.notification_api_client.NotificationApiClient.get') + NotificationApiClient().get_notifications_for_service('abcd1234', **arguments) + mock_get.assert_called_once_with(**expected_call) From a4a93116be2eaa89a32ad959235001020c5999af Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 2 Mar 2016 15:37:35 +0000 Subject: [PATCH 09/12] Put real notifications on job page --- app/__init__.py | 3 +++ app/main/views/jobs.py | 16 ++++++---------- app/templates/views/job.html | 10 ++-------- tests/app/main/views/test_send.py | 1 + 4 files changed, 12 insertions(+), 18 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index b9e070318..489a90ed9 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -14,6 +14,7 @@ from app.notify_client.api_client import NotificationsAdminAPIClient from app.notify_client.api_key_api_client import ApiKeyApiClient from app.notify_client.user_api_client import UserApiClient from app.notify_client.job_api_client import JobApiClient +from app.notify_client.notification_api_client import NotificationApiClient from app.notify_client.status_api_client import StatusApiClient from app.notify_client.permission_api_client import PermissionApiClient from app.notify_client.invite_api_client import InviteApiClient @@ -31,6 +32,7 @@ notifications_api_client = NotificationsAdminAPIClient() user_api_client = UserApiClient() api_key_api_client = ApiKeyApiClient() job_api_client = JobApiClient() +notification_api_client = NotificationApiClient() status_api_client = StatusApiClient() invite_api_client = InviteApiClient() asset_fingerprinter = AssetFingerprinter() @@ -50,6 +52,7 @@ def create_app(config_name, config_overrides=None): user_api_client.init_app(application) api_key_api_client.init_app(application) job_api_client.init_app(application) + notification_api_client.init_app(application) status_api_client.init_app(application) permission_api_client.init_app(application) invite_api_client.init_app(application) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 9469fa810..8268c83bb 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -10,7 +10,7 @@ from flask_login import login_required from notifications_python_client.errors import HTTPError from utils.template import Template -from app import job_api_client +from app import job_api_client, notification_api_client from app.main import main from app.main.dao import templates_dao from app.main.dao import services_dao @@ -41,18 +41,14 @@ 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 = [] + notifications = notification_api_client.get_notifications_for_service(service_id, job_id) return render_template( 'views/job.html', - messages=messages, + notifications=notifications['notifications'], counts={ - 'total': len(messages), - 'delivered': len([ - message for message in messages if message['status'] == 'Delivered' - ]), - 'failed': len([ - message for message in messages if message['status'] == 'Failed' - ]) + 'total': len(notifications), + 'delivered': len(notifications), + 'failed': 0 }, cost=u'£0.00', uploaded_file_name=job['original_file_name'], diff --git a/app/templates/views/job.html b/app/templates/views/job.html index b10e5d89f..6524e1a82 100644 --- a/app/templates/views/job.html +++ b/app/templates/views/job.html @@ -61,24 +61,18 @@
      {% call(item) list_table( - [ - {'row': 1, 'phone': '+447700 900995', 'template': template['name'], 'status': 'sent'} - ], + notifications, caption=uploaded_file_name, caption_visible=False, empty_message="Messages go here", field_headings=[ - 'Row', 'Recipient', 'Template', right_aligned_field_heading('Status') ] ) %} {% call field() %} - {{ item.row }}. - {% endcall %} - {% call field() %} - {{item.phone[:3]}} •••• •••••• + {{ item.to }} {% endcall %} {% call field() %} {{item.template}} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 9c7e2a30f..7e8b9e087 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -242,6 +242,7 @@ def test_create_job_should_call_api( job_data, mock_create_job, mock_get_job, + mock_get_notifications, mock_get_service, mock_get_service_template, mock_has_permissions From 3a3e4258ef35ed2e59584a91e5255b3badb65bc8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 2 Mar 2016 15:37:55 +0000 Subject: [PATCH 10/12] Remove template column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As one of our user research participants pointed out, this column is redundant because we’re showing the template on the page already. --- app/templates/views/job.html | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/templates/views/job.html b/app/templates/views/job.html index 6524e1a82..f62d6eaa6 100644 --- a/app/templates/views/job.html +++ b/app/templates/views/job.html @@ -74,9 +74,6 @@ {% call field() %} {{ item.to }} {% endcall %} - {% call field() %} - {{item.template}} - {% endcall %} {% call field( align='right', status='error' if item.status == 'Failed' else 'default' From d9073862fa0e0a250deef525766a4b85f352af61 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 2 Mar 2016 16:15:15 +0000 Subject: [PATCH 11/12] Put data from job on job page The main change is showing the finished time if the job is finished, rather than the start time. --- app/__init__.py | 7 ++++ app/main/views/jobs.py | 11 ++++-- app/templates/views/job.html | 62 ++++++++++++++++++------------- tests/__init__.py | 14 ++++++- tests/app/main/views/test_jobs.py | 3 +- tests/conftest.py | 11 ++++++ 6 files changed, 77 insertions(+), 31 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 489a90ed9..c4d7a25b1 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -73,6 +73,7 @@ def create_app(config_name, config_overrides=None): application.add_template_filter(nl2br) application.add_template_filter(format_datetime) + application.add_template_filter(format_time) application.add_template_filter(syntax_highlight_json) application.add_template_filter(valid_phone_number) @@ -149,6 +150,12 @@ def format_datetime(date): return native.strftime('%A %d %B %Y at %H:%M') +def format_time(date): + date = dateutil.parser.parse(date) + native = date.replace(tzinfo=None) + return native.strftime('%H:%M') + + def valid_phone_number(phone_number): try: validate_phone_number(phone_number) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 8268c83bb..bb0013a63 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -42,23 +42,26 @@ def view_job(service_id, job_id): try: job = job_api_client.get_job(service_id, job_id)['data'] notifications = notification_api_client.get_notifications_for_service(service_id, job_id) + finished = job['status'] == 'finished' return render_template( 'views/job.html', notifications=notifications['notifications'], counts={ - 'total': len(notifications), - 'delivered': len(notifications), + 'queued': 0 if finished else job['notification_count'], + 'sent': job['notification_count'] if finished else 0, 'failed': 0 }, + uploaded_at=job['created_at'], + finished_at=job['updated_at'] if finished else None, cost=u'£0.00', 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'], prefix=service['name'] ), service_id=service_id, - service=service + from_name=service['name'], + job_id=job_id ) except HTTPError as e: if e.status_code == 404: diff --git a/app/templates/views/job.html b/app/templates/views/job.html index f62d6eaa6..63346c7ea 100644 --- a/app/templates/views/job.html +++ b/app/templates/views/job.html @@ -28,23 +28,29 @@ template.subject, template, from_address='{}@notifications.service.gov.uk'.format(service.email_from), - from_name=service.name + from_name=from_name )}} {% endif %} -

      - Started {{ uploaded_file_time|format_datetime }} -

      + {% if finished_at %} +

      + Finished {{ finished_at|format_datetime }} +

      + {% else %} +

      + Started {{ uploaded_at|format_datetime }} +

      + {% endif %}
      • {{ big_number( - 0, 'queued' + counts.queued, 'queued' )}}
      • {{ big_number( - 1, 'sent' + counts.sent, 'sent' )}}
      • @@ -60,26 +66,32 @@
      - {% call(item) list_table( - notifications, - caption=uploaded_file_name, - caption_visible=False, - empty_message="Messages go here", - field_headings=[ - 'Recipient', - 'Template', - right_aligned_field_heading('Status') - ] - ) %} - {% call field() %} - {{ item.to }} - {% endcall %} - {% call field( - align='right', - status='error' if item.status == 'Failed' else 'default' + + {% if notifications %} + {% call(item) list_table( + notifications, + caption=uploaded_file_name, + caption_visible=False, + empty_message="Messages go here", + field_headings=[ + 'Recipient', + right_aligned_field_heading('Status') + ] ) %} - {{ item.status }} + {% call field() %} + {{ item.to }} + {% endcall %} + {% call field( + align='right', + status='error' if item.status == 'Failed' else 'default' + ) %} + {{ item.status|title }} at {{ item.sent_at|format_time }} + {% endcall %} {% endcall %} - {% endcall %} + {% else %} +

      + Refresh +

      + {% endif %} {% endblock %} diff --git a/tests/__init__.py b/tests/__init__.py index 57e3984fb..f5028b65b 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -109,6 +109,18 @@ def job_json(): 'bucket_name': 'service-1-{}-notify'.format(job_id), 'file_name': '{}.csv'.format(job_id), 'created_at': created_at, - 'notification_count': 1 + 'notification_count': 1, + 'status': '' } return data + + +def notification_json(): + import datetime + data = { + 'notifications': [{ + 'sent_at': str(datetime.datetime.now().time()) + } for i in range(5)], + 'links': {} + } + return data diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 6e5574061..8a8ff0aff 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -30,7 +30,8 @@ def test_should_show_page_for_one_job(app_, mock_get_service, mock_get_service_template, job_data, - mock_get_job): + mock_get_job, + mock_get_notifications): service_id = job_data['service'] job_id = job_data['id'] file_name = job_data['original_file_name'] diff --git a/tests/conftest.py b/tests/conftest.py index 24471d539..2779f849d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -10,6 +10,7 @@ from . import ( template_json, api_key_json, job_json, + notification_json, invite_json ) from app.notify_client.models import ( @@ -541,6 +542,16 @@ def mock_get_jobs(mocker): return mocker.patch('app.job_api_client.get_job', side_effect=_get_jobs) +@pytest.fixture(scope='function') +def mock_get_notifications(mocker): + def _get_notifications(service_id, job_id): + return notification_json() + return mocker.patch( + 'app.notification_api_client.get_notifications_for_service', + side_effect=_get_notifications + ) + + @pytest.fixture(scope='function') def mock_has_permissions(mocker): def _has_permission(permissions, service_id=None, or_=False): From 8287e1bad1fc010bb3a4c66997dd07fd364597e9 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Thu, 3 Mar 2016 12:15:24 +0000 Subject: [PATCH 12/12] Fix permission names, all tests passing. --- app/main/views/send.py | 17 +++++++++-------- app/main/views/service_settings.py | 16 ++++++++-------- app/templates/main_nav.html | 6 +++--- app/templates/views/choose-template.html | 4 ++-- 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index b622d526a..8f7aa93e9 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -31,7 +31,7 @@ from app.utils import ( from utils.process_csv import first_column_heading -manage_service_page_headings = { +send_messages_page_headings = { 'email': 'Send emails', 'sms': 'Send text messages' } @@ -44,8 +44,9 @@ manage_templates_page_headings = { def get_page_headings(template_type): - if current_user.has_permissions(['manage_service']): - return manage_service_page_headings[template_type] + # User has manage_service role + if current_user.has_permissions(['send_texts', 'send_emails', 'send_letters']): + return send_messages_page_headings[template_type] else: return manage_templates_page_headings[template_type] @@ -59,7 +60,7 @@ def letters_stub(service_id): @main.route("/services//send/", methods=['GET']) @login_required -@user_has_permissions('send_messages', 'manage_templates', or_=True) +@user_has_permissions('send_texts', 'send_emails', 'send_letters', 'manage_templates', or_=True) def choose_template(service_id, template_type): service = services_dao.get_service_by_id_or_404(service_id) @@ -92,7 +93,7 @@ def choose_template(service_id, template_type): @main.route("/services//send/", methods=['GET', 'POST']) @login_required -@user_has_permissions('send_messages') +@user_has_permissions('send_texts', 'send_emails', 'send_letters') def send_messages(service_id, template_id): form = CsvUploadForm() @@ -130,7 +131,7 @@ def send_messages(service_id, template_id): @main.route("/services//send/.csv", methods=['GET']) @login_required -@user_has_permissions('send_messages', 'manage_templates', or_=True) +@user_has_permissions('send_texts', 'send_emails', 'send_letters', 'manage_templates', or_=True) def get_example_csv(service_id, template_id): template = Template(templates_dao.get_service_template_or_404(service_id, template_id)['data']) output = io.StringIO() @@ -150,7 +151,7 @@ def get_example_csv(service_id, template_id): @main.route("/services//send//to-self", methods=['GET']) @login_required -@user_has_permissions('send_messages') +@user_has_permissions('send_texts', 'send_emails', 'send_letters') def send_message_to_self(service_id, template_id): template = Template(templates_dao.get_service_template_or_404(service_id, template_id)['data']) output = io.StringIO() @@ -186,7 +187,7 @@ def send_message_to_self(service_id, template_id): @main.route("/services//check/", methods=['GET', 'POST']) @login_required -@user_has_permissions('send_messages') +@user_has_permissions('send_texts', 'send_emails', 'send_letters') def check_messages(service_id, upload_id): upload_data = session['upload_data'] diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index df2b1f2f7..b813b4f5a 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -13,7 +13,7 @@ from notifications_python_client.errors import HTTPError @main.route("/services//service-settings") @login_required -@user_has_permissions('manage_service') +@user_has_permissions('manage_settings') def service_settings(service_id): try: service = get_service_by_id(service_id)['data'] @@ -31,7 +31,7 @@ def service_settings(service_id): @main.route("/services//service-settings/name", methods=['GET', 'POST']) @login_required -@user_has_permissions('manage_service') +@user_has_permissions('manage_settings') def service_name_change(service_id): try: service = get_service_by_id(service_id)['data'] @@ -56,7 +56,7 @@ def service_name_change(service_id): @main.route("/services//service-settings/name/confirm", methods=['GET', 'POST']) @login_required -@user_has_permissions('manage_service') +@user_has_permissions('manage_settings') def service_name_change_confirm(service_id): try: service = get_service_by_id(service_id)['data'] @@ -86,7 +86,7 @@ def service_name_change_confirm(service_id): @main.route("/services//service-settings/request-to-go-live", methods=['GET', 'POST']) @login_required -@user_has_permissions('manage_service') +@user_has_permissions('manage_settings') def service_request_to_go_live(service_id): try: service = get_service_by_id(service_id)['data'] @@ -109,7 +109,7 @@ def service_request_to_go_live(service_id): @main.route("/services//service-settings/status", methods=['GET', 'POST']) @login_required -@user_has_permissions('manage_service') +@user_has_permissions('manage_settings') def service_status_change(service_id): try: service = get_service_by_id(service_id)['data'] @@ -131,7 +131,7 @@ def service_status_change(service_id): @main.route("/services//service-settings/status/confirm", methods=['GET', 'POST']) @login_required -@user_has_permissions('manage_service') +@user_has_permissions('manage_settings') def service_status_change_confirm(service_id): try: service = get_service_by_id(service_id)['data'] @@ -160,7 +160,7 @@ def service_status_change_confirm(service_id): @main.route("/services//service-settings/delete", methods=['GET', 'POST']) @login_required -@user_has_permissions('manage_service') +@user_has_permissions('manage_settings') def service_delete(service_id): try: service = get_service_by_id(service_id)['data'] @@ -182,7 +182,7 @@ def service_delete(service_id): @main.route("/services//service-settings/delete/confirm", methods=['GET', 'POST']) @login_required -@user_has_permissions('manage_service') +@user_has_permissions('manage_settings') def service_delete_confirm(service_id): try: service = get_service_by_id(service_id)['data'] diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 96a360727..3f2727e40 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -2,7 +2,7 @@

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

      - {% if current_user.has_permissions(['send_messages']) %} + {% if current_user.has_permissions(['send_texts', 'send_emails', 'send_letters']) %} {% endif %} - {% if current_user.has_permissions(['manage_service']) %} + {% if current_user.has_permissions(['manage_users', 'manage_settings']) %} {% endif %} - {% if current_user.has_permissions(['manage_api_keys']) %} + {% if current_user.has_permissions(['manage_api_keys', 'access_developer_docs']) %}
      • API keys
      • Developer documentation
        • diff --git a/app/templates/views/choose-template.html b/app/templates/views/choose-template.html index bf6f08f12..00b4cbc2f 100644 --- a/app/templates/views/choose-template.html +++ b/app/templates/views/choose-template.html @@ -16,7 +16,7 @@ {% if templates %} {% if not has_jobs %} - {% if current_user.has_permissions(['manage_service']) %} + {% if current_user.has_permissions(['send_texts', 'send_emails', 'send_letters'], or_=True) %} {{ banner( """ Send yourself a test message @@ -41,7 +41,7 @@
        - {% if current_user.has_permissions(['manage_service']) %} + {% if current_user.has_permissions(['send_texts', 'send_emails', 'send_letters']) %} Add recipients Send yourself a test {% endif %}