From dbc57e3b583e38f5f593b950a0371e923344d039 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 3 Jun 2016 15:15:46 +0100 Subject: [PATCH 1/6] [WIP] use send_sms task to send sms code. Tests are broken because the template data for the Notify service is being delete after every test. Need a way to seed the data for the test. --- app/user/rest.py | 31 ++++++++++++------ tests/app/user/test_rest_verify.py | 50 ++++++++++++++++++++++-------- 2 files changed, 58 insertions(+), 23 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index 2d46d2d7a..1850c1c04 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -1,8 +1,8 @@ import json +import uuid from datetime import datetime from flask import (jsonify, request, abort, Blueprint, current_app) -from app import encryption - +from app import encryption, DATETIME_FORMAT from app.dao.users_dao import ( get_model_users, save_model_user, @@ -11,12 +11,12 @@ from app.dao.users_dao import ( use_user_code, increment_failed_login_count, reset_failed_login_count, - get_user_by_email + get_user_by_email, + create_secret_code ) - from app.dao.permissions_dao import permission_dao from app.dao.services_dao import dao_fetch_service_by_id - +from app.dao.templates_dao import dao_get_template_by_id from app.schemas import ( email_data_request_schema, user_schema, @@ -26,7 +26,7 @@ from app.schemas import ( ) from app.celery.tasks import ( - send_sms_code, + send_sms, email_reset_password, email_registration_verification ) @@ -123,14 +123,26 @@ def send_user_sms_code(user_id): if errors: return jsonify(result="error", message=errors), 400 - from app.dao.users_dao import create_secret_code secret_code = create_secret_code() create_user_code(user_to_send_to, secret_code, 'sms') mobile = user_to_send_to.mobile_number if verify_code.get('to', None) is None else verify_code.get('to') - verification_message = {'to': mobile, 'secret_code': secret_code} + sms_code_template_id = current_app.config['SMS_CODE_TEMPLATE_ID'] + sms_code_template = dao_get_template_by_id(sms_code_template_id) + verification_message = encryption.encrypt({ + 'template': sms_code_template_id, + 'template_version': sms_code_template.version, + 'to': mobile, + 'personalisation': { + 'verify_code': secret_code + } - send_sms_code.apply_async([encryption.encrypt(verification_message)], queue='sms-code') + }) + send_sms.apply_async([current_app.config['NOTIFY_SERVICE_ID'], + str(uuid.uuid4()), + verification_message, + datetime.utcnow().strftime(DATETIME_FORMAT) + ], queue='sms-code') return jsonify({}), 204 @@ -142,7 +154,6 @@ def send_user_email_verification(user_id): if errors: return jsonify(result="error", message=errors), 400 - from app.dao.users_dao import create_secret_code secret_code = create_secret_code() create_user_code(user_to_send_to, secret_code, 'email') diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index c426a3469..b437aee3c 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -6,8 +6,7 @@ from datetime import ( timedelta ) -from flask import url_for - +from flask import url_for, current_app from app.models import ( VerifyCode, User @@ -215,10 +214,13 @@ def test_user_verify_password_missing_password(notify_api, assert 'Required field missing data' in json_resp['message']['password'] +@freeze_time("2016-01-01 11:09:00.061258") def test_send_user_sms_code(notify_api, - sample_sms_code, - mock_celery_send_sms_code, - mock_encryption): + notify_db, + notify_db_session, + sample_user, + mock_encryption, + mocker): """ Tests POST endpoint /user//sms-code """ @@ -226,34 +228,56 @@ def test_send_user_sms_code(notify_api, with notify_api.test_client() as client: data = json.dumps({}) auth_header = create_authorization_header() + mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('uuid.uuid4', return_value='some_uuid') # for the notification id resp = client.post( - url_for('user.send_user_sms_code', user_id=sample_sms_code.user.id), + url_for('user.send_user_sms_code', user_id=sample_user.id), data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 - app.celery.tasks.send_sms_code.apply_async.assert_called_once_with(['something_encrypted'], - queue='sms-code') + app.celery.tasks.send_sms.apply_async.assert_called_once_with( + ([current_app.config['NOTIFY_SERVICE_ID'], + "some_uuid", + "something_encrypted", + "2016-01-01T11:09:00.061258"]), + queue="sms-code" + ) +@freeze_time("2016-01-01 11:09:00.061258") def test_send_user_code_for_sms_with_optional_to_field(notify_api, - sample_sms_code, + sample_user, mock_secret_code, - mock_celery_send_sms_code): + mocker): """ Tests POST endpoint '//code' successful sms with optional to field """ with notify_api.test_request_context(): with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('uuid.uuid4', return_value='some_uuid') # for the notification id data = json.dumps({'to': '+441119876757'}) auth_header = create_authorization_header() resp = client.post( - url_for('user.send_user_sms_code', user_id=sample_sms_code.user.id), + url_for('user.send_user_sms_code', user_id=sample_user.id), data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 - encrypted = encryption.encrypt({'to': '+441119876757', 'secret_code': '11111'}) - app.celery.tasks.send_sms_code.apply_async.assert_called_once_with([encrypted], queue='sms-code') + encrypted = encryption.encrypt({'template': current_app.config['SMS_CODE_TEMPLATE_ID'], + 'template_version': 1, + 'to': '+441119876757', + 'personalisation': { + 'verify_code': '11111' + } + }) + app.celery.tasks.send_sms.apply_async.assert_called_once_with( + ([current_app.config['NOTIFY_SERVICE_ID'], + "some_uuid", + encrypted, + "2016-01-01T11:09:00.061258"]), + queue="sms-code" + ) def test_send_sms_code_returns_404_for_bad_input_data(notify_api, notify_db, notify_db_session): From c2ae4773dac1ff617678928972c603acceec3496 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Jun 2016 10:32:24 +0100 Subject: [PATCH 2/6] Order templates by newest created first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When you add a new template, it’s probably the one that you want to do subsequent stuff with. But it’s also helpful to see the template in context (with its siblings) to understand that there are multiple templates. So we don’t want to do what we do in https://github.com/alphagov/notifications-admin/pull/648 for adding a new template. But we _can_ make your brand-new template appear first by always ordering by when the template was created. This also removes the confusion caused by having `updated_at` affecting order, and causing the templates to move around all the time. --- app/dao/templates_dao.py | 2 +- tests/app/dao/test_templates_dao.py | 9 +++++---- tests/app/template/test_rest.py | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index eba2212f1..d69b530e0 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -45,7 +45,7 @@ def dao_get_all_templates_for_service(service_id): service_id=service_id, archived=False ).order_by( - asc(Template.updated_at), asc(Template.created_at) + desc(Template.created_at) ).all() diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 445fbbba0..ad4b4372e 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -99,7 +99,7 @@ def test_get_all_templates_for_service(notify_db, notify_db_session, service_fac assert len(dao_get_all_templates_for_service(service_2.id)) == 2 -def test_get_all_templates_for_service_in_created_order(notify_db, notify_db_session, sample_service): +def test_get_all_templates_for_service_shows_newest_created_first(notify_db, notify_db_session, sample_service): template_1 = create_sample_template( notify_db, notify_db_session, @@ -126,13 +126,14 @@ def test_get_all_templates_for_service_in_created_order(notify_db, notify_db_ses ) assert Template.query.count() == 3 - assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'Sample Template 1' + assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'Sample Template 3' assert dao_get_all_templates_for_service(sample_service.id)[1].name == 'Sample Template 2' - assert dao_get_all_templates_for_service(sample_service.id)[2].name == 'Sample Template 3' + assert dao_get_all_templates_for_service(sample_service.id)[2].name == 'Sample Template 1' template_2.name = 'Sample Template 2 (updated)' dao_update_template(template_2) - assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'Sample Template 2 (updated)' + assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'Sample Template 3' + assert dao_get_all_templates_for_service(sample_service.id)[1].name == 'Sample Template 2 (updated)' def test_get_all_returns_empty_list_if_no_templates(sample_service): diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 45e7300e6..25e03d390 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -250,10 +250,10 @@ def test_should_be_able_to_get_all_templates_for_a_service(notify_api, sample_us assert response.status_code == 200 update_json_resp = json.loads(response.get_data(as_text=True)) - assert update_json_resp['data'][0]['name'] == 'my template 1' + assert update_json_resp['data'][0]['name'] == 'my template 2' assert update_json_resp['data'][0]['version'] == 1 assert update_json_resp['data'][0]['created_at'] - assert update_json_resp['data'][1]['name'] == 'my template 2' + assert update_json_resp['data'][1]['name'] == 'my template 1' assert update_json_resp['data'][1]['version'] == 1 assert update_json_resp['data'][1]['created_at'] From 34740c582725d2556598296fd79415c85d89a27e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Jun 2016 10:59:38 +0100 Subject: [PATCH 3/6] Bump utils to latest version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit doesn’t introduce any new functionality, just keeps things from getting too far behind. This is a breaking change in utils, see: https://github.com/alphagov/notifications-utils/commit/4683922d30704e932e7a49d5c3ab058814ee5356 None of these methods are used by the API, so no API code needs to change, cf: https://github.com/alphagov/notifications-api/search?utf8=%E2%9C%93&q=rows_with_errors&type=Code https://github.com/alphagov/notifications-api/search?utf8=%E2%9C%93&q=has_errors&type=Code https://github.com/alphagov/notifications-api/search?utf8=%E2%9C%93&q=annotated_rows&type=Code https://github.com/alphagov/notifications-api/search?utf8=%E2%9C%93&q=annotated_rows_with_errors&type=Code --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 7526f9afc..595a22d39 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,4 +22,4 @@ statsd==3.2.1 git+https://github.com/alphagov/notifications-python-client.git@1.0.0#egg=notifications-python-client==1.0.0 -git+https://github.com/alphagov/notifications-utils.git@5.2.8#egg=notifications-utils==5.2.8 +git+https://github.com/alphagov/notifications-utils.git@6.2.0#egg=notifications-utils==6.2.0 From be9fde1420f7e8c46318c1f204d19f183cdba53d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 6 Jun 2016 11:51:12 +0100 Subject: [PATCH 4/6] Fix tests for sending sms codes. Since the unit tests delete the data in between tests I need to add the template data for the test for send sms code. --- tests/app/conftest.py | 44 ++++++++++++++++++++++++------ tests/app/user/test_rest_verify.py | 27 ++++++++++++------ 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 8dc6cae4b..27ef72278 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -4,6 +4,7 @@ import uuid from datetime import (datetime, date) import pytest +from flask import current_app from app import db from app.models import ( @@ -307,15 +308,6 @@ def sample_email_job(notify_db, return job -@pytest.fixture(scope='function') -def mock_secret_code(mocker): - def _create(): - return '11111' - - mock_class = mocker.patch('app.dao.users_dao.create_secret_code', side_effect=_create) - return mock_class - - @pytest.fixture(scope='function') def sample_notification(notify_db, notify_db_session, @@ -563,3 +555,37 @@ def mock_mmg_client(mocker, statsd_client=None): }) client.init_app(current_app, statsd_client) return client + + +@pytest.fixture(scope='function') +def sms_code_template(notify_db, + notify_db_session): + user = sample_user(notify_db, notify_db_session) + service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) + if not service: + data = { + 'id': current_app.config['NOTIFY_SERVICE_ID'], + 'name': 'Notify Service', + 'message_limit': 1000, + 'active': True, + 'restricted': False, + 'email_from': 'notify.service', + 'created_by': user + } + service = Service(**data) + db.session.add(service) + + template = Template.query.get(current_app.config['SMS_CODE_TEMPLATE_ID']) + if not template: + data = { + 'id': current_app.config['SMS_CODE_TEMPLATE_ID'], + 'name': 'Sms code template', + 'template_type': 'sms', + 'content': '((verify_code))', + 'service': service, + 'created_by': user, + 'archived': False + } + template = Template(**data) + db.session.add(template) + return template diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index b437aee3c..3ca379171 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -216,18 +216,18 @@ def test_user_verify_password_missing_password(notify_api, @freeze_time("2016-01-01 11:09:00.061258") def test_send_user_sms_code(notify_api, - notify_db, - notify_db_session, sample_user, - mock_encryption, + sms_code_template, mocker): """ Tests POST endpoint /user//sms-code """ + with notify_api.test_request_context(): with notify_api.test_client() as client: data = json.dumps({}) auth_header = create_authorization_header() + mocked = mocker.patch('app.user.rest.create_secret_code', return_value='11111') mocker.patch('app.celery.tasks.send_sms.apply_async') mocker.patch('uuid.uuid4', return_value='some_uuid') # for the notification id resp = client.post( @@ -235,10 +235,18 @@ def test_send_user_sms_code(notify_api, data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 + assert mocked.call_count == 1 + encrypted = encryption.encrypt({'template': current_app.config['SMS_CODE_TEMPLATE_ID'], + 'template_version': 1, + 'to': sample_user.mobile_number, + 'personalisation': { + 'verify_code': '11111' + } + }) app.celery.tasks.send_sms.apply_async.assert_called_once_with( ([current_app.config['NOTIFY_SERVICE_ID'], "some_uuid", - "something_encrypted", + encrypted, "2016-01-01T11:09:00.061258"]), queue="sms-code" ) @@ -247,15 +255,17 @@ def test_send_user_sms_code(notify_api, @freeze_time("2016-01-01 11:09:00.061258") def test_send_user_code_for_sms_with_optional_to_field(notify_api, sample_user, - mock_secret_code, + sms_code_template, + mock_encryption, mocker): """ - Tests POST endpoint '//code' successful sms with optional to field - """ + Tests POST endpoint '//code' successful sms with optional to field + """ with notify_api.test_request_context(): with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_sms.apply_async') + mocked = mocker.patch('app.user.rest.create_secret_code', return_value='11111') mocker.patch('uuid.uuid4', return_value='some_uuid') # for the notification id + mocker.patch('app.celery.tasks.send_sms.apply_async') data = json.dumps({'to': '+441119876757'}) auth_header = create_authorization_header() resp = client.post( @@ -271,6 +281,7 @@ def test_send_user_code_for_sms_with_optional_to_field(notify_api, 'verify_code': '11111' } }) + assert mocked.call_count == 1 app.celery.tasks.send_sms.apply_async.assert_called_once_with( ([current_app.config['NOTIFY_SERVICE_ID'], "some_uuid", From 63c29a3a3d07cabe86e772abd71c1b84bebcaa1e Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Mon, 6 Jun 2016 12:37:06 +0100 Subject: [PATCH 5/6] API recieves full set of permissions names on create of user invite. This is instead of mapping from permission groups to individual permissions on user creation. --- app/dao/permissions_dao.py | 2 ++ app/permissions_utils.py | 28 ---------------------------- app/service/rest.py | 10 +++++----- tests/app/service/test_rest.py | 24 +++++++++++++++++++----- 4 files changed, 26 insertions(+), 38 deletions(-) delete mode 100644 app/permissions_utils.py diff --git a/app/dao/permissions_dao.py b/app/dao/permissions_dao.py index eca2446e9..7ff9bb447 100644 --- a/app/dao/permissions_dao.py +++ b/app/dao/permissions_dao.py @@ -68,6 +68,8 @@ class PermissionDAO(DAOClass): query = self.get_query(filter_by_dict={'user': user.id, 'service': service.id}) query.delete() for p in permissions: + p.user = user + p.service = service self.create_instance(p, _commit=False) except Exception as e: if _commit: diff --git a/app/permissions_utils.py b/app/permissions_utils.py deleted file mode 100644 index fa9268523..000000000 --- a/app/permissions_utils.py +++ /dev/null @@ -1,28 +0,0 @@ -from app.models import ( - MANAGE_USERS, - MANAGE_TEMPLATES, - MANAGE_SETTINGS, - SEND_TEXTS, - SEND_EMAILS, - SEND_LETTERS, - MANAGE_API_KEYS, - VIEW_ACTIVITY -) - -from app.schemas import permission_schema - - -permissions_groups = {'send_messages': [SEND_TEXTS, SEND_EMAILS, SEND_LETTERS], - 'manage_service': [MANAGE_USERS, MANAGE_SETTINGS, MANAGE_TEMPLATES], - 'manage_api_keys': [MANAGE_API_KEYS], - VIEW_ACTIVITY: [VIEW_ACTIVITY]} - - -def get_permissions_by_group(permission_groups): - requested_permissions = [] - for group in permission_groups: - permissions = permissions_groups[group] - for permission in permissions: - requested_permissions.append({'permission': permission}) - permissions, errors = permission_schema.load(requested_permissions, many=True) - return permissions diff --git a/app/service/rest.py b/app/service/rest.py index 1e28886b8..80b82fe67 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -26,12 +26,13 @@ from app.dao.services_dao import ( from app.dao.provider_statistics_dao import get_fragment_count from app.dao.users_dao import get_model_users -from app.models import ApiKey + from app.schemas import ( service_schema, api_key_schema, user_schema, - from_to_date_schema + from_to_date_schema, + permission_schema ) from app.errors import register_errors @@ -150,10 +151,9 @@ def add_user_to_service(service_id, user_id): return jsonify(result='error', message='User id: {} already part of service id: {}'.format(user_id, service_id)), 400 - permissions_json = request.get_json().get('permissions', []) - permissions = _process_permissions(user, service, permissions_json) - dao_add_user_to_service(service, user, permissions) + permissions, errors = permission_schema.load(request.get_json(), many=True) + dao_add_user_to_service(service, user, permissions) data, errors = service_schema.dump(service) return jsonify(data=data), 201 diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 7c8b52c37..7dc43853a 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -640,7 +640,14 @@ def test_add_existing_user_to_another_service_with_all_permissions(notify_api, # they must exist in db first save_model_user(user_to_add) - data = {'permissions': ['send_messages', 'manage_service', 'manage_api_keys']} + data = [{"permission": "send_emails"}, + {"permission": "send_letters"}, + {"permission": "send_texts"}, + {"permission": "manage_users"}, + {"permission": "manage_settings"}, + {"permission": "manage_api_keys"}, + {"permission": "manage_templates"}, + {"permission": "view_activity"}] auth_header = create_authorization_header() @@ -672,7 +679,7 @@ def test_add_existing_user_to_another_service_with_all_permissions(notify_api, json_resp = json.loads(resp.get_data(as_text=True)) permissions = json_resp['data']['permissions'][str(sample_service.id)] expected_permissions = ['send_texts', 'send_emails', 'send_letters', 'manage_users', - 'manage_settings', 'manage_templates', 'manage_api_keys'] + 'manage_settings', 'manage_templates', 'manage_api_keys', 'view_activity'] assert sorted(expected_permissions) == sorted(permissions) @@ -693,7 +700,10 @@ def test_add_existing_user_to_another_service_with_send_permissions(notify_api, ) save_model_user(user_to_add) - data = {'permissions': ['send_messages']} + data = [{"permission": "send_emails"}, + {"permission": "send_letters"}, + {"permission": "send_texts"}] + auth_header = create_authorization_header() resp = client.post( @@ -734,7 +744,10 @@ def test_add_existing_user_to_another_service_with_manage_permissions(notify_api ) save_model_user(user_to_add) - data = {'permissions': ['manage_service']} + data = [{"permission": "manage_users"}, + {"permission": "manage_settings"}, + {"permission": "manage_templates"}] + auth_header = create_authorization_header() resp = client.post( @@ -775,7 +788,8 @@ def test_add_existing_user_to_another_service_with_manage_api_keys(notify_api, ) save_model_user(user_to_add) - data = {'permissions': ['manage_api_keys']} + data = [{"permission": "manage_api_keys"}] + auth_header = create_authorization_header() resp = client.post( From 4d072cb1130b59b292437a9b1acc468996813cff Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 7 Jun 2016 09:55:10 +0100 Subject: [PATCH 6/6] Rename notify service to GOV.UK Notify --- .../versions/0026_rename_notify_service.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 migrations/versions/0026_rename_notify_service.py diff --git a/migrations/versions/0026_rename_notify_service.py b/migrations/versions/0026_rename_notify_service.py new file mode 100644 index 000000000..2070aff84 --- /dev/null +++ b/migrations/versions/0026_rename_notify_service.py @@ -0,0 +1,28 @@ +"""empty message + +Revision ID: 0026_rename_notify_service +Revises: 0025_notify_service_data +Create Date: 2016-06-07 09:51:07.343334 + +""" + +# revision identifiers, used by Alembic. +revision = '0026_rename_notify_service' +down_revision = '0025_notify_service_data' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.get_bind() + op.execute("update services set name = 'GOV.UK Notify' where id = 'd6aa2c68-a2d9-4437-ab19-3ae8eb202553'") + op.execute("update services_history set name = 'GOV.UK Notify' where id = 'd6aa2c68-a2d9-4437-ab19-3ae8eb202553'") + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + pass + ### end Alembic commands ###