From 5f3c72729ef247c240a8883437319dc6da3d21e5 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Mon, 29 Feb 2016 17:35:21 +0000 Subject: [PATCH 1/3] [WIP] Start of user accepting invite. This commit only deals with acceptance by users who are already in system. Changed invite client to return invited user objects instead of dictionaries. Added commented out test. fixed up fixtures to return invited user object for invites --- app/main/__init__.py | 23 ++++- app/main/views/invites.py | 34 +++++++ app/main/views/manage_users.py | 4 +- app/notify_client/invite_api_client.py | 21 ++++- app/notify_client/models.py | 15 ++++ app/notify_client/user_api_client.py | 5 ++ app/templates/views/manage-users.html | 6 +- tests/__init__.py | 7 +- tests/app/main/views/test_accept_invite.py | 25 ++++++ tests/app/main/views/test_manage_users.py | 100 ++++++++++----------- tests/conftest.py | 59 ++++++++---- 11 files changed, 220 insertions(+), 79 deletions(-) create mode 100644 app/main/views/invites.py create mode 100644 tests/app/main/views/test_accept_invite.py diff --git a/app/main/__init__.py b/app/main/__init__.py index ec9691d24..39f95b71d 100644 --- a/app/main/__init__.py +++ b/app/main/__init__.py @@ -3,7 +3,24 @@ from flask import Blueprint main = Blueprint('main', __name__) from app.main.views import ( - index, sign_in, sign_out, register, two_factor, verify, send, add_service, - code_not_received, jobs, dashboard, templates, service_settings, forgot_password, - new_password, styleguide, user_profile, choose_service, api_keys, manage_users + index, + sign_in, + sign_out, + register, + two_factor, + verify, + send, + add_service, + code_not_received, + jobs, dashboard, + templates, + service_settings, + forgot_password, + new_password, + styleguide, + user_profile, + choose_service, + api_keys, + manage_users, + invites ) diff --git a/app/main/views/invites.py b/app/main/views/invites.py new file mode 100644 index 000000000..4b271e29c --- /dev/null +++ b/app/main/views/invites.py @@ -0,0 +1,34 @@ +from flask import ( + redirect, + url_for, + abort +) + +from notifications_python_client.errors import HTTPError + +from app.main import main +from app import ( + invite_api_client, + user_api_client +) + + +@main.route("/invitation/") +def accept_invite(token): + try: + invited_user = invite_api_client.accept_invite(token) + existing_user = user_api_client.get_user_by_email(invited_user['email_address']) + + if existing_user: + user_api_client.add_user_to_service(invited_user['service'], + existing_user.id, + invited_user) + return redirect(url_for('main.service_dashboard', service_id=invited_user['service'])) + else: + # TODO implement registration flow for new users + abort(404) + except HTTPError as e: + if e.status_code == 404: + abort(404) + else: + raise e diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 0b5f0e7f0..f5d83bfd6 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -58,8 +58,8 @@ def invite_user(service_id): email_address = form.email_address.data permissions = _get_permissions(request.form) try: - resp = invite_api_client.create_invite(current_user.id, service_id, email_address, permissions) - flash('Invite sent to {}'.format(resp['email_address']), 'default_with_tick') + invited_user = invite_api_client.create_invite(current_user.id, service_id, email_address, permissions) + flash('Invite sent to {}'.format(invited_user.email_address), 'default_with_tick') return redirect(url_for('.manage_users', service_id=service_id)) except HTTPError as e: diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index 1dffed425..d1acd5f6d 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -1,6 +1,5 @@ - from notifications_python_client.base import BaseAPIClient -from app.notify_client.models import User +from app.notify_client.models import InvitedUser class InviteApiClient(BaseAPIClient): @@ -22,9 +21,23 @@ class InviteApiClient(BaseAPIClient): 'permissions': permissions } resp = self.post(url='/service/{}/invite'.format(service_id), data=data) - return resp['data'] + return InvitedUser(**resp['data']) def get_invites_for_service(self, service_id): endpoint = '/service/{}/invite'.format(service_id) resp = self.get(endpoint) - return [User(data) for data in resp['data']] + invites = resp['data'] + invited_users = _get_invited_users(invites) + return invited_users + + def accept_invite(self, token): + resp = self.get(url='/invite/{}'.format(token)) + return InvitedUser(**resp['data']) + + +def _get_invited_users(invites): + invited_users = [] + for invite in invites: + invited_user = InvitedUser(**invite) + invited_users.append(invited_user) + return invited_users diff --git a/app/notify_client/models.py b/app/notify_client/models.py index 986caf570..d5caf5b33 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -110,3 +110,18 @@ class User(UserMixin): def set_password(self, pwd): self._password = pwd + + +class InvitedUser(object): + + def __init__(self, id, service, from_user, email_address, permissions, status, created_at): + self.id = id + self.service = str(service) + self.from_user = from_user + self.email_address = email_address + self.permissions = permissions.split(',') + self.status = status + self.created_at = created_at + + def has_permissions(self, permission): + return permission in self.permissions diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 179a7d4f8..5bb744a91 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -85,3 +85,8 @@ class UserApiClient(BaseAPIClient): endpoint = '/service/{}/users'.format(service_id) resp = self.get(endpoint) return [User(data) for data in resp['data']] + + def add_user_to_service(self, service_id, user_id, invited_user): + endpoint = '/service/{}/users/{}'.format(service_id, user_id) + resp = self.post(endpoint, data=invited_user) + return User(resp['data'], max_failed_login_count=self.max_failed_login_count) diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index 02d3d817e..b3adee4be 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -43,9 +43,9 @@ Manage users – GOV.UK Notify {% call field() %} {{ item.email_address }} {% endcall %} - {{ boolean_field(item.has_permissions(service_id, 'send_messages')) }} - {{ boolean_field(item.has_permissions(service_id, 'manage_service')) }} - {{ boolean_field(item.has_permissions(service_id, 'api_keys')) }} + {{ boolean_field(item.has_permissions('send_messages')) }} + {{ boolean_field(item.has_permissions('manage_service')) }} + {{ boolean_field(item.has_permissions('api_keys')) }} {% call field(align='right') %} Change {% endcall %} diff --git a/tests/__init__.py b/tests/__init__.py index 4792e8c7e..57e3984fb 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -48,12 +48,15 @@ def api_key_json(id_, name, expiry_date=None): } -def invite_json(id, from_user, service_id, email_address): +def invite_json(id, from_user, service_id, email_address, permissions, created_at): return {'id': id, 'from_user': from_user, 'service': service_id, 'email_address': email_address, - 'status': 'pending'} + 'status': 'pending', + 'permissions': permissions, + 'created_at': created_at + } TEST_USER_EMAIL = 'test@user.gov.uk' diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py new file mode 100644 index 000000000..8c52400c4 --- /dev/null +++ b/tests/app/main/views/test_accept_invite.py @@ -0,0 +1,25 @@ +from flask import url_for + + +def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard(app_, + service_one, + sample_invite, + mock_accept_invite, + mock_get_user_by_email, + mock_add_user_to_service): + + expected_service = service_one['id'] + expected_from_user = service_one['users'][0] + expected_redirect_location = 'http://localhost/services/{}/dashboard'.format(expected_service) + + with app_.test_request_context(): + with app_.test_client() as client: + + response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) + + mock_accept_invite.assert_called_with('thisisnotarealtoken') + mock_get_user_by_email.assert_called_with('invited_user@test.gov.uk') + mock_add_user_to_service.assert_called_with(expected_service, expected_from_user, sample_invite) + + assert response.status_code == 302 + assert response.location == expected_redirect_location diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 4af60ee7c..438e3060e 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -3,22 +3,22 @@ from flask import url_for from bs4 import BeautifulSoup -# def test_should_show_overview_page( -# app_, -# api_user_active, -# mock_login, -# mock_get_service, -# mock_get_users_by_service, -# mock_get_invites_for_service -# ): -# with app_.test_request_context(): -# with app_.test_client() as client: -# client.login(api_user_active) -# response = client.get(url_for('main.manage_users', service_id=55555)) -# -# assert 'Manage team' in response.get_data(as_text=True) -# assert response.status_code == 200 -# mock_get_users_by_service.assert_called_once_with(service_id='55555') +def test_should_show_overview_page( + app_, + api_user_active, + mock_login, + mock_get_service, + mock_get_users_by_service, + mock_get_invites_for_service +): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + response = client.get(url_for('main.manage_users', service_id=55555)) + + assert 'Manage team' in response.get_data(as_text=True) + assert response.status_code == 200 + mock_get_users_by_service.assert_called_once_with(service_id='55555') def test_should_show_page_for_one_user( @@ -70,37 +70,37 @@ def test_should_show_page_for_inviting_user( assert 'Add a new team member' in response.get_data(as_text=True) assert response.status_code == 200 -# -# def test_invite_user( -# app_, -# service_one, -# api_user_active, -# mock_login, -# mock_get_users_by_service, -# mock_create_invite, -# mock_get_invites_for_service -# ): -# from_user = api_user_active.id -# service_id = service_one['id'] -# email_address = 'test@example.gov.uk' -# permissions = 'send_messages,manage_service,manage_api_keys' -# -# with app_.test_request_context(): -# with app_.test_client() as client: -# client.login(api_user_active) -# response = client.post( -# url_for('main.invite_user', service_id=service_id), -# data={'email_address': email_address, -# 'send_messages': 'yes', -# 'manage_service': 'yes', -# 'manage_api_keys': 'yes'}, -# follow_redirects=True -# ) -# -# assert response.status_code == 200 -# mock_create_invite.assert_called_with(from_user, service_id, email_address, permissions) -# mock_get_invites_for_service.assert_called_with(service_id=service_id) -# page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') -# assert page.h1.string.strip() == 'Manage team' -# flash_banner = page.find('div', class_='banner-default-with-tick').string.strip() -# assert flash_banner == 'Invite sent to test@example.gov.uk' + +def test_invite_user( + app_, + service_one, + api_user_active, + mock_login, + mock_get_users_by_service, + mock_create_invite, + mock_get_invites_for_service +): + from_user = api_user_active.id + service_id = service_one['id'] + email_address = 'test@example.gov.uk' + permissions = 'send_messages,manage_service,manage_api_keys' + + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + response = client.post( + url_for('main.invite_user', service_id=service_id), + data={'email_address': email_address, + 'send_messages': 'yes', + 'manage_service': 'yes', + 'manage_api_keys': 'yes'}, + follow_redirects=True + ) + + assert response.status_code == 200 + mock_create_invite.assert_called_with(from_user, service_id, email_address, permissions) + mock_get_invites_for_service.assert_called_with(service_id=service_id) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Manage team' + flash_banner = page.find('div', class_='banner-default-with-tick').string.strip() + assert flash_banner == 'Invite sent to test@example.gov.uk' diff --git a/tests/conftest.py b/tests/conftest.py index db5bdfa2c..b04b81e14 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -12,7 +12,10 @@ from . import ( job_json, invite_json ) -from app.notify_client.models import User +from app.notify_client.models import ( + User, + InvitedUser +) @pytest.fixture(scope='session') @@ -551,27 +554,53 @@ def mock_s3_upload(mocker): @pytest.fixture(scope='function') -def mock_create_invite(mocker): +def sample_invite(mocker, service_one): + import datetime + id = str(uuid.uuid4()) + from_user = service_one['users'][0] + email_address = 'invited_user@test.gov.uk' + service_id = service_one['id'] + permissions = 'send_messages,manage_service,manage_api_keys' + created_at = datetime.datetime.now() + return invite_json(id, from_user, service_id, email_address, permissions, created_at) + + +@pytest.fixture(scope='function') +def mock_create_invite(mocker, sample_invite): + def _create_invite(from_user, service_id, email_address, permissions): - data = {'id': uuid.uuid4(), - 'from_user': from_user, - 'service': service_id, - 'email_address': email_address, - 'status': 'pending', - 'permissions': permissions} - return data + sample_invite['from_user'] = from_user + sample_invite['service'] = service_id + sample_invite['email_address'] = email_address + sample_invite['status'] = 'pending' + sample_invite['permissions'] = permissions + return InvitedUser(**sample_invite) return mocker.patch('app.invite_api_client.create_invite', side_effect=_create_invite) @pytest.fixture(scope='function') -def mock_get_invites_for_service(mocker, service_one): +def mock_get_invites_for_service(mocker, service_one, sample_invite): + import copy + def _get_invites(service_id): data = [] - from_user = service_one['users'][0] - service_id = service_one['id'] for i in range(0, 5): - email_address = 'user_{}@testnotify.gov.uk'.format(i) - invite = invite_json(uuid.uuid4(), from_user, service_id, email_address) - data.append(invite) + invite = copy.copy(sample_invite) + invite['email_address'] = 'user_{}@testnotify.gov.uk'.format(i) + data.append(InvitedUser(**invite)) return data return mocker.patch('app.invite_api_client.get_invites_for_service', side_effect=_get_invites) + + +@pytest.fixture(scope='function') +def mock_accept_invite(mocker, sample_invite): + def _accept_token(token): + return sample_invite + return mocker.patch('app.invite_api_client.accept_invite', side_effect=_accept_token) + + +@pytest.fixture(scope='function') +def mock_add_user_to_service(mocker, service_one): + def _add_user(service_id, user_id, invitation): + return api_user_active + return mocker.patch('app.user_api_client.add_user_to_service', side_effect=_add_user) From 5f02d4cefeb84f469b095eb9352fef9a45b525cb Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 1 Mar 2016 17:23:23 +0000 Subject: [PATCH 2/3] [WIP] Post does not need any data Invites rest module can use invited user object instead of dict. --- app/main/views/invites.py | 10 +++++----- app/notify_client/user_api_client.py | 6 ++++-- app/templates/views/manage-users.html | 2 +- tests/app/main/views/test_accept_invite.py | 9 +++++++-- tests/conftest.py | 6 +++--- 5 files changed, 20 insertions(+), 13 deletions(-) diff --git a/app/main/views/invites.py b/app/main/views/invites.py index 4b271e29c..c9052303d 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -17,13 +17,13 @@ from app import ( def accept_invite(token): try: invited_user = invite_api_client.accept_invite(token) - existing_user = user_api_client.get_user_by_email(invited_user['email_address']) + existing_user = user_api_client.get_user_by_email(invited_user.email_address) if existing_user: - user_api_client.add_user_to_service(invited_user['service'], - existing_user.id, - invited_user) - return redirect(url_for('main.service_dashboard', service_id=invited_user['service'])) + user_api_client.add_user_to_service(invited_user.service, + existing_user.id) + + return redirect(url_for('main.service_dashboard', service_id=invited_user.service)) else: # TODO implement registration flow for new users abort(404) diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 5bb744a91..b1054f769 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -1,3 +1,5 @@ +import json + from notifications_python_client.notifications import BaseAPIClient from notifications_python_client.errors import HTTPError @@ -86,7 +88,7 @@ class UserApiClient(BaseAPIClient): resp = self.get(endpoint) return [User(data) for data in resp['data']] - def add_user_to_service(self, service_id, user_id, invited_user): + def add_user_to_service(self, service_id, user_id): endpoint = '/service/{}/users/{}'.format(service_id, user_id) - resp = self.post(endpoint, data=invited_user) + resp = self.post(endpoint) return User(resp['data'], max_failed_login_count=self.max_failed_login_count) diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index b3adee4be..44a0b39e5 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -45,7 +45,7 @@ Manage users – GOV.UK Notify {% endcall %} {{ boolean_field(item.has_permissions('send_messages')) }} {{ boolean_field(item.has_permissions('manage_service')) }} - {{ boolean_field(item.has_permissions('api_keys')) }} + {{ boolean_field(item.has_permissions('manage_api_keys')) }} {% call field(align='right') %} Change {% endcall %} diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 8c52400c4..54b527d93 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -1,15 +1,20 @@ from flask import url_for +from app.notify_client.models import InvitedUser +from notifications_python_client.errors import HTTPError + +import pytest + def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard(app_, service_one, + api_user_active, sample_invite, mock_accept_invite, mock_get_user_by_email, mock_add_user_to_service): expected_service = service_one['id'] - expected_from_user = service_one['users'][0] expected_redirect_location = 'http://localhost/services/{}/dashboard'.format(expected_service) with app_.test_request_context(): @@ -19,7 +24,7 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard(app_, mock_accept_invite.assert_called_with('thisisnotarealtoken') mock_get_user_by_email.assert_called_with('invited_user@test.gov.uk') - mock_add_user_to_service.assert_called_with(expected_service, expected_from_user, sample_invite) + mock_add_user_to_service.assert_called_with(expected_service, api_user_active.id) assert response.status_code == 302 assert response.location == expected_redirect_location diff --git a/tests/conftest.py b/tests/conftest.py index b04b81e14..acd163be9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -595,12 +595,12 @@ def mock_get_invites_for_service(mocker, service_one, sample_invite): @pytest.fixture(scope='function') def mock_accept_invite(mocker, sample_invite): def _accept_token(token): - return sample_invite + return InvitedUser(**sample_invite) return mocker.patch('app.invite_api_client.accept_invite', side_effect=_accept_token) @pytest.fixture(scope='function') -def mock_add_user_to_service(mocker, service_one): - def _add_user(service_id, user_id, invitation): +def mock_add_user_to_service(mocker, service_one, api_user_active): + def _add_user(service_id, user_id): return api_user_active return mocker.patch('app.user_api_client.add_user_to_service', side_effect=_add_user) From 98169550d8b714fd7d6056106f128d59a4f244db Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 1 Mar 2016 17:32:04 +0000 Subject: [PATCH 3/3] [WIP] no data needed for post to create add user to service --- app/notify_client/user_api_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index b1054f769..d6a4ad574 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -90,5 +90,5 @@ class UserApiClient(BaseAPIClient): def add_user_to_service(self, service_id, user_id): endpoint = '/service/{}/users/{}'.format(service_id, user_id) - resp = self.post(endpoint) + resp = self.post(endpoint, data={}) return User(resp['data'], max_failed_login_count=self.max_failed_login_count)