From ba0c9ac6c11727d9121b0a3b7fc145bbf66a86ac Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 19 Jan 2016 11:15:00 +0000 Subject: [PATCH 1/6] Remove placeholder logs of notifications sent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the: - dashboard - activity page This info will be confusing for users at the hack day, because it will say they’ve already sent messages when they first sign up. This involved changing the table macro to have a nice ‘no rows’ message. --- app/assets/stylesheets/components/table.scss | 7 ++++++ app/main/views/dashboard.py | 2 +- app/main/views/jobs.py | 2 +- app/templates/components/table.html | 23 ++++++++++---------- app/templates/views/jobs.html | 1 + app/templates/views/service_dashboard.html | 8 ++----- app/templates/views/styleguide.html | 15 +++++++++++++ tests/app/main/views/test_dashboard.py | 3 +-- tests/app/main/views/test_jobs.py | 3 +-- 9 files changed, 41 insertions(+), 23 deletions(-) diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index 37478ad6d..ed749b6e3 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -41,3 +41,10 @@ @extend .table-field; text-align: right; } + +.table-empty-message { + @include core-16; + color: $secondary-text-colour; + border-bottom: 1px solid $border-colour; + padding: 5px 0 8px 0; +} diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 1c8842d4a..419e2cca5 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -19,6 +19,6 @@ def service_dashboard(service_id): return render_template( 'views/service_dashboard.html', jobs=jobs, - free_text_messages_remaining=560, + free_text_messages_remaining='25,000', spent_this_month='0.00', service_id=service_id) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index c548cb44c..ae653d405 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -50,7 +50,7 @@ messages = [ def view_jobs(service_id): return render_template( 'views/jobs.html', - jobs=jobs, + jobs=[], # use `jobs` for placeholder data service_id=service_id ) diff --git a/app/templates/components/table.html b/app/templates/components/table.html index bfa53232d..fb41a6aa7 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -23,20 +23,21 @@ {%- endmacro %} {% macro list_table(items, caption='', empty_message='', field_headings=[], field_headings_visible=True, caption_visible=True) -%} - {% if items %} - {% set parent_caller = caller %} - {% call mapping_table(caption, field_headings, field_headings_visible, caption_visible) %} - {% for item in items %} - {% call row() %} - {{ parent_caller(item) }} - {% endcall %} - {% endfor %} - {%- endcall %} - {% else %} -

+ + {% set parent_caller = caller %} + {% call mapping_table(caption, field_headings, field_headings_visible, caption_visible) %} + {% for item in items %} + {% call row() %} + {{ parent_caller(item) }} + {% endcall %} + {% endfor %} + {%- endcall %} + {% if not items %} +

{{ empty_message }}

{% endif %} + {%- endmacro %} {% macro row() -%} diff --git a/app/templates/views/jobs.html b/app/templates/views/jobs.html index 0879fee7a..6d321c433 100644 --- a/app/templates/views/jobs.html +++ b/app/templates/views/jobs.html @@ -13,6 +13,7 @@ GOV.UK Notify | Notifications activity jobs, caption="Recent activity", caption_visible=False, + empty_message='You haven’t sent any notifications yet', field_headings=['Job', 'File', 'Time', 'Status'] ) %} {% call field() %} diff --git a/app/templates/views/service_dashboard.html b/app/templates/views/service_dashboard.html index 9e5510795..9ed330140 100644 --- a/app/templates/views/service_dashboard.html +++ b/app/templates/views/service_dashboard.html @@ -24,9 +24,9 @@ {% call(item) list_table( - jobs[:3], + [], caption="Recent text messages", - empty_message="No recent text messages", + empty_message='You haven’t sent any text messages yet', field_headings=['Job', 'File', 'Time', 'Status'] ) %} {% call field() %} @@ -42,9 +42,5 @@ {{ item.status }} {% endcall %} {% endcall %} -

- See all notifications activity -

- {% endblock %} diff --git a/app/templates/views/styleguide.html b/app/templates/views/styleguide.html index 2231b1173..40669cbbf 100644 --- a/app/templates/views/styleguide.html +++ b/app/templates/views/styleguide.html @@ -148,6 +148,21 @@ {% endcall %} {% endcall %} + {% call(item) list_table( + [], + caption='Jobs', + field_headings=['Job', 'Time'], + caption_visible=True, + empty_message='You haven’t scheduled any jobs yet' + ) %} + {% call field() %} + {{ item.job }} + {% endcall %} + {% call field() %} + {{ item.time }} + {% endcall %} + {% endcall %} +

Textbox

{{ textbox(form.username) }} {{ textbox(form.password) }} diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index e0d163f99..4af4b63ac 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -13,5 +13,4 @@ def test_should_show_recent_jobs_on_dashboard(app_, response = client.get(url_for('main.service_dashboard', service_id=123)) assert response.status_code == 200 - assert 'Test message 1' in response.get_data(as_text=True) - assert 'Asdfgg' in response.get_data(as_text=True) + assert 'You haven’t sent any text messages yet' in response.get_data(as_text=True) diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 039d47dbf..aa900dd8b 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -11,8 +11,7 @@ def test_should_return_list_of_all_jobs(app_, db_, db_session, service_one): response = client.get(url_for('main.view_jobs', service_id=101)) assert response.status_code == 200 - assert 'Test message 1' in response.get_data(as_text=True) - assert 'Final reminder' in response.get_data(as_text=True) + assert 'You haven’t sent any notifications yet' in response.get_data(as_text=True) def test_should_show_page_for_one_job(app_, db_, db_session, service_one): From cfb3f96b01d656850fbadbb933951193dcf83354 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Tue, 19 Jan 2016 15:54:12 +0000 Subject: [PATCH 2/6] Mapped template actions to the api and mocked tests. --- app/main/dao/services_dao.py | 6 +- app/main/dao/templates_dao.py | 49 +++++++++ app/main/forms.py | 12 ++- app/main/views/templates.py | 123 +++++++++++++--------- app/notify_client/api_client.py | 14 +++ app/templates/main_nav.html | 2 +- app/templates/views/edit-template.html | 8 +- app/templates/views/job.html | 2 +- app/templates/views/manage-templates.html | 12 +-- app/templates/views/send-sms.html | 2 +- tests/__init__.py | 10 ++ tests/app/main/views/test_templates.py | 103 +++++++++++++++--- tests/conftest.py | 66 +++++++++++- 13 files changed, 321 insertions(+), 88 deletions(-) create mode 100644 app/main/dao/templates_dao.py diff --git a/app/main/dao/services_dao.py b/app/main/dao/services_dao.py index aea05e075..cf4c096be 100644 --- a/app/main/dao/services_dao.py +++ b/app/main/dao/services_dao.py @@ -1,9 +1,5 @@ from flask import url_for -from datetime import datetime -from client.errors import HTTPError, InvalidResponse -from sqlalchemy.orm import load_only -from flask.ext.login import current_user -from app import (db, notifications_api_client) +from app import notifications_api_client from app.main.utils import BrowsableItem diff --git a/app/main/dao/templates_dao.py b/app/main/dao/templates_dao.py new file mode 100644 index 000000000..461af615c --- /dev/null +++ b/app/main/dao/templates_dao.py @@ -0,0 +1,49 @@ +from flask import url_for +from app import notifications_api_client +from app.main.utils import BrowsableItem + + +def insert_service_template(name, type_, content, service_id): + return notifications_api_client.create_service_template( + name, type_, content, service_id) + + +def update_service_template(id_, name, type_, content, service_id): + return notifications_api_client.update_service_template( + id_, name, type_, content, service_id) + + +def get_service_templates(service_id): + return notifications_api_client.get_service_templates(service_id) + + +def get_service_template(service_id, template_id): + return notifications_api_client.get_service_template( + service_id, template_id) + + +def delete_service_template(service_id, template_id): + return notifications_api_client.delete_service_template( + service_id, template_id) + + +class TemplatesBrowsableItem(BrowsableItem): + + @property + def title(self): + return self._item['name'] + + @property + def link(self): + return url_for( + 'main.edit_service_template', + service_id=self._item['service'], + template_id=self._item['id']) + + @property + def destructive(self): + return False + + @property + def hint(self): + return "Some service template hint here" diff --git a/app/main/forms.py b/app/main/forms.py index 11b201402..df9515a5c 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -6,7 +6,8 @@ from wtforms import ( PasswordField, ValidationError, TextAreaField, - FileField + FileField, + SelectField ) from wtforms.validators import DataRequired, Email, Length, Regexp @@ -187,8 +188,13 @@ class ConfirmPasswordForm(Form): class TemplateForm(Form): - template_name = StringField(u'Template name') - template_body = TextAreaField(u'Message') + name = StringField( + u'Template name', + validators=[DataRequired(message="Template name cannot be empty")]) + template_type = SelectField(u'Template type', choices=[('sms', 'SMS')]) + content = TextAreaField( + u'Message', + validators=[DataRequired(message="Template content cannot be empty")]) class ForgotPasswordForm(Form): diff --git a/app/main/views/templates.py b/app/main/views/templates.py index fdba437fe..f075601e9 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -3,80 +3,99 @@ from flask_login import login_required from app.main import main from app.main.forms import TemplateForm - -from ._templates import templates +from app.main.dao.services_dao import get_service_by_id +from app.main.dao import templates_dao as dao +from client.errors import HTTPError @main.route("/services//templates") @login_required -def manage_templates(service_id): +def manage_service_templates(service_id): + try: + templates = dao.get_service_templates(service_id)['data'] + except HTTPError as e: + if e.status_code == 404: + abort(404) + else: + raise e return render_template( 'views/manage-templates.html', service_id=service_id, - templates=templates, - ) + templates=templates) @main.route("/services//templates/add", methods=['GET', 'POST']) @login_required -def add_template(service_id): +def add_service_template(service_id): + try: + service = dao.get_service_by_id(service_id)['data'] + except HTTPError as e: + if e.status_code == 404: + abort(404) + else: + raise e form = TemplateForm() - if request.method == 'GET': - return render_template( - 'views/edit-template.html', - h1='Add template', - form=form, - service_id=service_id - ) - elif request.method == 'POST': - return redirect(url_for('.manage_templates', service_id=service_id)) + if form.validate_on_submit(): + dao.insert_service_template( + form.name.data, form.template_type.data, form.content.data, service_id) + return redirect(url_for( + '.manage_service_templates', service_id=service_id)) + return render_template( + 'views/edit-template.html', + h1='Add template', + form=form, + service_id=service_id) @main.route("/services//templates/", methods=['GET', 'POST']) @login_required -def edit_template(service_id, template_id): +def edit_service_template(service_id, template_id): + try: + template = dao.get_service_template(service_id, template_id)['data'] + except HTTPError as e: + if e.status_code == 404: + abort(404) + else: + raise e + form = TemplateForm(form_data=template) - form = TemplateForm() + if form.validate_on_submit(): + dao.update_service_template( + template_id, form.name.data, form.template_type.data, + form.content.data, service_id) + return redirect(url_for('.manage_service_templates', service_id=service_id)) - form.template_name.data = templates[template_id - 1]['name'] - form.template_body.data = templates[template_id - 1]['body'] - - if request.method == 'GET': - return render_template( - 'views/edit-template.html', - h1='Edit template', - form=form, - service_id=service_id, - template_id=template_id - ) - elif request.method == 'POST': - return redirect(url_for('.manage_templates', service_id=service_id)) + return render_template( + 'views/edit-template.html', + h1='Edit template', + form=form, + service_id=service_id, + template_id=template_id) @main.route("/services//templates//delete", methods=['GET', 'POST']) @login_required -def delete_template(service_id, template_id): - - form = TemplateForm() - - form.template_name.data = templates[template_id - 1]['name'] - form.template_body.data = templates[template_id - 1]['body'] - - if request.method == 'GET': - - flash('Are you sure you want to delete ‘{}’?'.format(form.template_name.data), 'delete') - - return render_template( - 'views/edit-template.html', - h1='Edit template', - form=form, - service_id=service_id, - template_id=template_id - ) - elif request.method == 'POST': - if request.form.get('delete'): - return redirect(url_for('.manage_templates', service_id=service_id)) +def delete_service_template(service_id, template_id): + try: + template = dao.get_service_template(service_id, template_id)['data'] + except HTTPError as e: + if e.status_code == 404: + abort(404) else: - return redirect(url_for('.manage_templates', service_id=service_id)) + raise e + + form = TemplateForm(form_data=template) + + if form.validate_on_submit(): + dao.delete_service_template(service_id, template_id) + return redirect(url_for('.manage_service_templates', service_id=service_id)) + + flash('Are you sure you want to delete ‘{}’?'.format(form.name.data), 'delete') + return render_template( + 'views/edit-template.html', + h1='Edit template', + form=form, + service_id=service_id, + template_id=template_id) diff --git a/app/notify_client/api_client.py b/app/notify_client/api_client.py index 50217721b..8da2174db 100644 --- a/app/notify_client/api_client.py +++ b/app/notify_client/api_client.py @@ -83,6 +83,20 @@ class NotificationsAdminAPIClient(NotificationsAPIClient): endpoint = "/service/{0}/template".format(service_id) return self.post(endpoint, data) + def update_service_template(self, id_, name, type_, content, service_id): + """ + Update a service template. + """ + data = { + 'id': id_, + 'name': name, + 'template_type': type_, + 'content': content, + 'service': service_id + } + endpoint = "/service/{0}/template/{1}".format(service_id, id_) + return self.put(endpoint, data) + def get_service_template(self, service_id, template_id, *params): """ Retrieve a service template. diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index d4000984b..65cc28c57 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -6,7 +6,7 @@
  • Send text messages
  • Send emails
  • Activity
    • -
  • Templates
    • +
  • Templates
    • API keys and documentation
      • diff --git a/app/templates/views/edit-template.html b/app/templates/views/edit-template.html index 20cc74482..07155a454 100644 --- a/app/templates/views/edit-template.html +++ b/app/templates/views/edit-template.html @@ -11,12 +11,12 @@ GOV.UK Notify | Edit template

      {{ h1 }}

      - {{ textbox(form.template_name) }} - {{ textbox(form.template_body, highlight_tags=True) }} + {{ textbox(form.name) }} + {{ textbox(form.content, highlight_tags=True) }} {{ page_footer( 'Save', - delete_link=url_for('.delete_template', service_id=service_id, template_id=template_id) if template_id or None, - back_link=url_for('.manage_templates', service_id=service_id), + delete_link=url_for('.delete_service_template', service_id=service_id, template_id=template_id) if template_id or None, + back_link=url_for('.manage_service_templates', service_id=service_id), back_link_text='Back to templates' ) }}
      diff --git a/app/templates/views/job.html b/app/templates/views/job.html index b5970e947..01916ea24 100644 --- a/app/templates/views/job.html +++ b/app/templates/views/job.html @@ -38,7 +38,7 @@ GOV.UK Notify | Notifications activity

    - Sent with template {{ template_used }} at {{ uploaded_file_time }} + Sent with template {{ template_used }} at {{ uploaded_file_time }}

    {% call(item) list_table( diff --git a/app/templates/views/manage-templates.html b/app/templates/views/manage-templates.html index dbdbed51f..50e541959 100644 --- a/app/templates/views/manage-templates.html +++ b/app/templates/views/manage-templates.html @@ -15,21 +15,21 @@ GOV.UK Notify | Manage templates

    Templates

    - Create new template + Create new template {% for template in templates %} {% if template.type == 'sms' %} {{ sms_message( - template.body, + template.content, name=template.name, - edit_link=url_for('.edit_template', service_id=service_id, template_id=loop.index) + edit_link=url_for('.edit_service_template', service_id=template.service, template_id=template.id) ) }} {% elif template.type == 'email' %} {{ email_message( - template.subject, - template.body, + "template subject", + template.content, name=template.name, - edit_link=url_for('.edit_template', service_id=service_id, template_id=loop.index) + edit_link=url_for('.edit_service_template', service_id=template.service, template_id=template.id) ) }} {% endif %} {% endfor %} diff --git a/app/templates/views/send-sms.html b/app/templates/views/send-sms.html index ebfa317cc..338497756 100644 --- a/app/templates/views/send-sms.html +++ b/app/templates/views/send-sms.html @@ -23,7 +23,7 @@

    - or create a new template + or create a new template

    2. Add recipients

    diff --git a/tests/__init__.py b/tests/__init__.py index 4bd8718e9..339886b10 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -29,6 +29,16 @@ def service_json(id_, name, users, limit=1000, active=False, restricted=True): } +def template_json(id_, name, type_, content, service_id): + return { + 'id': id_, + 'name': name, + 'template_type': type_, + 'content': content, + 'service': service_id + } + + TEST_USER_EMAIL = 'test@user.gov.uk' diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index da0489c7d..d010be260 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -1,49 +1,128 @@ +import json from flask import url_for -def test_should_return_list_of_all_templates(app_, db_, db_session, active_user): +def test_should_return_list_of_all_templates(app_, + db_, + db_session, + active_user, + mock_get_service_templates): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) - response = client.get(url_for('.manage_templates', service_id=123)) + service_id = 123 + response = client.get(url_for( + '.manage_service_templates', service_id=service_id)) assert response.status_code == 200 + mock_get_service_templates.assert_called_with(service_id) -def test_should_show_page_for_one_templates(app_, db_, db_session, active_user): +def test_should_show_page_for_one_templates(app_, + db_, + db_session, + active_user, + mock_get_service_template): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) - response = client.get(url_for('.edit_template', service_id=123, template_id=1)) + service_id = 123 + template_id = 456 + response = client.get(url_for( + '.edit_service_template', + service_id=service_id, + template_id=template_id)) assert response.status_code == 200 + mock_get_service_template.assert_called_with( + service_id, template_id) -def test_should_redirect_when_saving_a_template(app_, db_, db_session, active_user): +def test_should_redirect_when_saving_a_template(app_, + db_, + db_session, + active_user, + mock_get_service_template, + mock_update_service_template): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) - response = client.post(url_for('.edit_template', service_id=123, template_id=1)) + service_id = 123 + template_id = 456 + name = "new name" + type_ = "sms" + content = "template content" + data = { + 'id': template_id, + 'name': name, + 'template_type': type_, + "content": content, + "service": service_id + } + response = client.post(url_for( + '.edit_service_template', + service_id=service_id, + template_id=template_id), data=data) assert response.status_code == 302 - assert response.location == url_for('.manage_templates', service_id=123, _external=True) + assert response.location == url_for( + '.manage_service_templates', service_id=service_id, _external=True) + mock_update_service_template.assert_called_with( + template_id, name, type_, content, service_id) -def test_should_show_delete_template_page(app_, db_, db_session, active_user): +def test_should_show_delete_template_page(app_, + db_, + db_session, + active_user, + mock_get_service_template): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) - response = client.get(url_for('.delete_template', service_id=123, template_id=1)) + service_id = 123 + template_id = 456 + response = client.get(url_for( + '.delete_service_template', + service_id=service_id, + template_id=template_id)) assert response.status_code == 200 assert 'Are you sure' in response.get_data(as_text=True) + mock_get_service_template.assert_called_with( + service_id, template_id) -def test_should_redirect_when_deleting_a_template(app_, db_, db_session, active_user): +def test_should_redirect_when_deleting_a_template(app_, + db_, + db_session, + active_user, + mock_get_service_template, + mock_delete_service_template): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) - response = client.post(url_for('.delete_template', service_id=123, template_id=1)) + service_id = 123 + template_id = 456 + name = "new name" + type_ = "sms" + content = "template content" + data = { + 'id': template_id, + 'name': name, + 'template_type': type_, + 'content': content, + 'service': service_id + } + response = client.post(url_for( + '.delete_service_template', + service_id=service_id, + template_id=template_id), data=data) assert response.status_code == 302 - assert response.location == url_for('.manage_templates', service_id=123, _external=True) + assert response.location == url_for( + '.manage_service_templates', + service_id=service_id, _external=True) + mock_get_service_template.assert_called_with( + service_id, template_id) + mock_delete_service_template.assert_called_with( + service_id, template_id) diff --git a/tests/conftest.py b/tests/conftest.py index 7776cd767..48df70174 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,7 +7,7 @@ from flask.ext.script import Manager from sqlalchemy.schema import MetaData from . import ( create_test_user, create_another_test_user, service_json, TestClient, - get_test_user) + get_test_user, template_json) from app import create_app, db @@ -94,12 +94,11 @@ def mock_get_service(mocker, active_user): @pytest.fixture(scope='function') def mock_create_service(mocker): - # TODO fix token generation def _create(service_name, active, limit, restricted, user_id): service = service_json( 101, service_name, [user_id], limit=limit, active=active, restricted=restricted) - return {'data': service, 'token': 1} + return {'data': service} mock_class = mocker.patch( 'app.notifications_api_client.create_service', side_effect=_create) return mock_class @@ -142,3 +141,64 @@ def mock_delete_service(mocker, mock_get_service): mock_class = mocker.patch( 'app.notifications_api_client.delete_service', side_effect=_delete) return mock_class + + +@pytest.fixture(scope='function') +def mock_get_service_template(mocker): + def _create(service_id, template_id): + template = template_json( + template_id, "Template Name", "sms", "template content", service_id) + return {'data': template} + return mocker.patch( + 'app.notifications_api_client.get_service_template', + side_effect=_create) + + +@pytest.fixture(scope='function') +def mock_create_service_template(mocker): + def _create(name, type_, content, service): + template = template_json( + 101, name, type_, content, service) + return {'data': template} + mock_class = mocker.patch( + 'app.notifications_api_client.create_service_template', + side_effect=_create) + return mock_class + + +@pytest.fixture(scope='function') +def mock_update_service_template(mocker): + def _update(id_, name, type_, content, service): + template = template_json( + id_, name, type_, content, service) + return {'data': template} + mock_class = mocker.patch( + 'app.notifications_api_client.update_service_template', + side_effect=_update) + return mock_class + + +@pytest.fixture(scope='function') +def mock_get_service_templates(mocker): + def _create(service_id): + template_one = template_json( + 1, "template_one", "sms", "template one content", service_id) + template_two = template_json( + 2, "template_two", "sms", "template two content", service_id) + return {'data': [template_one, template_two]} + mock_class = mocker.patch( + 'app.notifications_api_client.get_service_templates', + side_effect=_create) + return mock_class + + +@pytest.fixture(scope='function') +def mock_delete_service_template(mocker): + def _delete(service_id, template_id): + template = template_json( + template_id, "Template to delete", + "sms", "content to be deleted", service_id) + return {'data': template} + mock_class = mocker.patch( + 'app.notifications_api_client.delete_service_template', side_effect=_delete) + return mock_class From e98eacf66181dc62dad2c8ef14704f8a2b130404 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 19 Jan 2016 16:02:24 +0000 Subject: [PATCH 3/6] Add admin client user name and secret (used to call into the API) --- config.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/config.py b/config.py index 9f98b37aa..c105f093d 100644 --- a/config.py +++ b/config.py @@ -26,6 +26,9 @@ class Config(object): NOTIFY_API_SECRET = os.getenv('NOTIFY_API_SECRET', "dev-secret") NOTIFY_API_CLIENT = os.getenv('NOTIFY_API_CLIENT', "admin") + ADMIN_CLIENT_USER_NAME = os.getenv('ADMIN_CLIENT_USER_NAME') + ADMIN_CLIENT_SECRET = os.getenv('ADMIN_CLIENT_SECRET') + WTF_CSRF_ENABLED = True SECRET_KEY = 'secret-key' HTTP_PROTOCOL = 'http' From bc7ad75899ac32757fe80415d640bb7f47405d32 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 19 Jan 2016 16:04:08 +0000 Subject: [PATCH 4/6] Add default config --- config.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config.py b/config.py index c105f093d..4dba047ff 100644 --- a/config.py +++ b/config.py @@ -41,6 +41,8 @@ class Config(object): class Development(Config): DEBUG = True + ADMIN_CLIENT_USER_NAME = 'dev-notify-admin' + ADMIN_CLIENT_SECRET = 'dev-notify-secret-key' class Test(Config): From c86b53f7f13b9264de9b1e639f8bcdc2edadfe74 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 19 Jan 2016 15:50:31 +0000 Subject: [PATCH 5/6] Add user via api Leave original user object in for this slice. Remove on next --- app/main/views/register.py | 30 ++++++++++++++++++++-- app/notify_client/user_api_client.py | 18 ++++++++++++++ config.py | 6 ++--- tests/app/main/views/test_register.py | 36 +++++++++++++++++++-------- tests/conftest.py | 14 +++++++++++ 5 files changed, 89 insertions(+), 15 deletions(-) create mode 100644 app/notify_client/user_api_client.py diff --git a/app/main/views/register.py b/app/main/views/register.py index 2ca7d8faa..c2856c1e6 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -1,12 +1,21 @@ from datetime import datetime, timedelta -from flask import render_template, redirect, session +from flask import ( + render_template, + redirect, + session, + current_app, + abort +) + +from client.errors import HTTPError from app.main import main +from app.models import User from app.main.dao import users_dao from app.main.forms import RegisterUserForm -from app.models import User +from app.notify_client.user_api_client import UserApiClient # TODO how do we handle duplicate unverifed email addresses? # malicious or otherwise. @@ -18,6 +27,8 @@ def register(): form = RegisterUserForm(users_dao.get_user_by_email) if form.validate_on_submit(): + + # TODO remove once all api integrations done user = User(name=form.name.data, email_address=form.email_address.data, mobile_number=form.mobile_number.data, @@ -25,6 +36,21 @@ def register(): created_at=datetime.now(), role_id=1) users_dao.insert_user(user) + + user_api_client = UserApiClient(current_app.config['NOTIFY_API_URL'], + current_app.config['ADMIN_CLIENT_USER_NAME'], + current_app.config['ADMIN_CLIENT_SECRET']) + try: + user_api_client.register_user(form.name.data, + form.email_address.data, + form.mobile_number.data, + form.password.data) + except HTTPError as e: + if e.status_code == 404: + abort(404) + else: + raise e + # TODO possibly there should be some exception handling # for sending sms and email codes. # How do we report to the user there is a problem with diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py new file mode 100644 index 000000000..bc6a70096 --- /dev/null +++ b/app/notify_client/user_api_client.py @@ -0,0 +1,18 @@ +from client.notifications import BaseAPIClient + + +class UserApiClient(BaseAPIClient): + + def __init__(self, base_url, client_id, secret): + super(self.__class__, self).__init__(base_url=base_url, + client_id=client_id, + secret=secret) + + def register_user(self, name, email_address, mobile_number, password): + data = { + "name": name, + "email_address": email_address, + "mobile_number": mobile_number, + "password": password} + + return self.post("/user", data) diff --git a/config.py b/config.py index 4dba047ff..6d3fcd2d9 100644 --- a/config.py +++ b/config.py @@ -22,7 +22,7 @@ class Config(object): SESSION_COOKIE_HTTPONLY = True SESSION_COOKIE_SECURE = True - NOTIFY_API_URL = os.getenv('NOTIFY_API_URL', "http://localhost:6001") + NOTIFY_API_URL = os.getenv('NOTIFY_API_URL') NOTIFY_API_SECRET = os.getenv('NOTIFY_API_SECRET', "dev-secret") NOTIFY_API_CLIENT = os.getenv('NOTIFY_API_CLIENT', "admin") @@ -41,12 +41,12 @@ class Config(object): class Development(Config): DEBUG = True + NOTIFY_API_URL = 'http://localhost:6011' ADMIN_CLIENT_USER_NAME = 'dev-notify-admin' ADMIN_CLIENT_SECRET = 'dev-notify-secret-key' -class Test(Config): - DEBUG = True +class Test(Development): SQLALCHEMY_DATABASE_URI = 'postgresql://localhost/test_notifications_admin' WTF_CSRF_ENABLED = False diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index e23362fcf..2a3c78536 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -1,5 +1,7 @@ from flask import url_for +from tests.conftest import mock_register_user + def test_render_register_returns_template_with_form(app_, db_, db_session): response = app_.test_client().get('/register') @@ -12,13 +14,21 @@ def test_process_register_creates_new_user(app_, db_, db_session, mock_send_sms, - mock_send_email): + mock_send_email, + mocker): + + user_data = { + 'name': 'Some One Valid', + 'email_address': 'someone@example.gov.uk', + 'mobile_number': '+4407700900460', + 'password': 'validPassword!' + } + + mock_register_user(mocker, user_data) + with app_.test_request_context(): response = app_.test_client().post('/register', - data={'name': 'Some One Valid', - 'email_address': 'someone@example.gov.uk', - 'mobile_number': '+4407700900460', - 'password': 'validPassword!'}) + data=user_data) assert response.status_code == 302 assert response.location == url_for('main.verify', _external=True) @@ -57,13 +67,19 @@ def test_should_add_verify_codes_on_session(app_, db_, db_session, mock_send_sms, - mock_send_email): + mock_send_email, + mocker): + user_data = { + 'name': 'Test Codes', + 'email_address': 'test@example.gov.uk', + 'mobile_number': '+4407700900460', + 'password': 'validPassword!' + } + + mock_register_user(mocker, user_data) with app_.test_client() as client: response = client.post('/register', - data={'name': 'Test Codes', - 'email_address': 'test_codes@example.gov.uk', - 'mobile_number': '+4407700900460', - 'password': 'validPassword!'}) + data=user_data) assert response.status_code == 302 assert 'notify_admin_session' in response.headers.get('Set-Cookie') diff --git a/tests/conftest.py b/tests/conftest.py index 7776cd767..3f476eae9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -142,3 +142,17 @@ def mock_delete_service(mocker, mock_get_service): mock_class = mocker.patch( 'app.notifications_api_client.delete_service', side_effect=_delete) return mock_class + + +@pytest.fixture(scope='function') +def mock_register_user(mocker, user_data): + data = { + "email_address": user_data['email_address'], + "failed_login_count": 0, + "mobile_number": user_data['mobile_number'], + "name": user_data['name'], + "state": "pending" + } + mock_class = mocker.patch('app.main.views.register.UserApiClient') + mock_class.register_user.return_value = data + return mock_class From 28658c6cc4c3fa3fa5a95073b1d99fc98387b5da Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 19 Jan 2016 16:39:04 +0000 Subject: [PATCH 6/6] Rename NOTIFY_API_URL to API_HOST_NAME. The NOTIFY_API_URL will point to the alpha api. --- app/main/views/register.py | 2 +- app/notify_client/api_client.py | 2 +- config.py | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/main/views/register.py b/app/main/views/register.py index c2856c1e6..7b4100cd8 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -37,7 +37,7 @@ def register(): role_id=1) users_dao.insert_user(user) - user_api_client = UserApiClient(current_app.config['NOTIFY_API_URL'], + user_api_client = UserApiClient(current_app.config['API_HOST_NAME'], current_app.config['ADMIN_CLIENT_USER_NAME'], current_app.config['ADMIN_CLIENT_SECRET']) try: diff --git a/app/notify_client/api_client.py b/app/notify_client/api_client.py index 50217721b..c3f91175a 100644 --- a/app/notify_client/api_client.py +++ b/app/notify_client/api_client.py @@ -12,7 +12,7 @@ class NotificationsAdminAPIClient(NotificationsAPIClient): "secret") def init_app(self, application): - self.base_url = application.config['NOTIFY_API_URL'] + self.base_url = application.config['API_HOST_NAME'] self.client_id = application.config['NOTIFY_API_CLIENT'] self.secret = application.config['NOTIFY_API_SECRET'] diff --git a/config.py b/config.py index 6d3fcd2d9..eb417cd7e 100644 --- a/config.py +++ b/config.py @@ -22,7 +22,7 @@ class Config(object): SESSION_COOKIE_HTTPONLY = True SESSION_COOKIE_SECURE = True - NOTIFY_API_URL = os.getenv('NOTIFY_API_URL') + API_HOST_NAME = os.getenv('API_HOST_NAME') NOTIFY_API_SECRET = os.getenv('NOTIFY_API_SECRET', "dev-secret") NOTIFY_API_CLIENT = os.getenv('NOTIFY_API_CLIENT', "admin") @@ -41,7 +41,7 @@ class Config(object): class Development(Config): DEBUG = True - NOTIFY_API_URL = 'http://localhost:6011' + API_HOST_NAME = 'http://localhost:6011' ADMIN_CLIENT_USER_NAME = 'dev-notify-admin' ADMIN_CLIENT_SECRET = 'dev-notify-secret-key'