diff --git a/app/main/views/invites.py b/app/main/views/invites.py index 02f9c644f..29ee21d1f 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -46,7 +46,6 @@ def accept_invite(token): return redirect(url_for('main.broadcast_tour', service_id=service.id, step_index=1)) return redirect(url_for('main.service_dashboard', service_id=invited_user.service)) - session['invited_user'] = invited_user.serialize() session['invited_user_id'] = invited_user.id existing_user = User.from_email_address_or_none(invited_user.email_address) @@ -108,7 +107,6 @@ def accept_org_invite(token): session.pop('invited_org_user_id', None) return redirect(url_for('main.organisation_dashboard', org_id=invited_org_user.organisation)) - session['invited_org_user'] = invited_org_user.serialize() session['invited_org_user_id'] = invited_org_user.id existing_user = User.from_email_address_or_none(invited_org_user.email_address) diff --git a/app/models/user.py b/app/models/user.py index bce03d7a9..43229ea72 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -522,11 +522,7 @@ class InvitedUser(JSONModel): @classmethod def from_session(cls): invited_user_id = session.get('invited_user_id') - if invited_user_id: - return cls.by_id(invited_user_id) - - invited_user = session.get('invited_user') - return cls(invited_user) if invited_user else None + return cls.by_id(invited_user_id) if invited_user_id else None def has_permissions(self, *permissions): if self.status == 'cancelled': @@ -606,11 +602,7 @@ class InvitedOrgUser(JSONModel): @classmethod def from_session(cls): invited_org_user_id = session.get('invited_org_user_id') - if invited_org_user_id: - return cls.by_id(invited_org_user_id) - - invited_org_user = session.get('invited_org_user') - return cls(invited_org_user) if invited_org_user else None + return cls.by_id(invited_org_user_id) if invited_org_user_id else None @classmethod def by_id_and_org_id(cls, org_id, invited_user_id): diff --git a/tests/app/main/views/organisations/test_organisation_invites.py b/tests/app/main/views/organisations/test_organisation_invites.py index 9de398170..22231eb89 100644 --- a/tests/app/main/views/organisations/test_organisation_invites.py +++ b/tests/app/main/views/organisations/test_organisation_invites.py @@ -229,11 +229,11 @@ def test_registration_from_org_invite_has_bad_data( client, sample_org_invite, data, - error + error, + mock_get_invited_org_user_by_id, ): - invited_org_user = InvitedOrgUser(sample_org_invite) with client.session_transaction() as session: - session['invited_org_user'] = invited_org_user.serialize() + session['invited_org_user_id'] = sample_org_invite['id'] response = client.post(url_for('main.register_from_org_invite'), data=data) @@ -249,22 +249,23 @@ def test_registration_from_org_invite_has_bad_data( def test_registration_from_org_invite_has_different_email_or_organisation( client, sample_org_invite, - diff_data + diff_data, + mock_get_invited_org_user_by_id, ): - invited_org_user = InvitedOrgUser(sample_org_invite) with client.session_transaction() as session: - session['invited_org_user'] = invited_org_user.serialize() + session['invited_org_user_id'] = sample_org_invite['id'] - for data in diff_data: - session['invited_org_user'][data] = 'different' - - response = client.post(url_for('main.register_from_org_invite'), data={ + data = { 'name': 'Test User', 'mobile_number': '+4407700900460', 'password': 'validPassword!', - 'email_address': session['invited_org_user']['email_address'], - 'organisation': session['invited_org_user']['organisation'] - }) + 'email_address': sample_org_invite['email_address'], + 'organisation': sample_org_invite['organisation'] + } + for field in diff_data: + data[field] = 'different' + + response = client.post(url_for('main.register_from_org_invite'), data=data) assert response.status_code == 400 @@ -276,25 +277,25 @@ def test_org_user_registers_with_email_already_in_use( mock_accept_org_invite, mock_add_user_to_organisation, mock_send_already_registered_email, - mock_register_user + mock_register_user, + mock_get_invited_org_user_by_id, ): - invited_org_user = InvitedOrgUser(sample_org_invite) with client.session_transaction() as session: - session['invited_org_user'] = invited_org_user.serialize() + session['invited_org_user_id'] = sample_org_invite['id'] response = client.post(url_for('main.register_from_org_invite'), data={ 'name': 'Test User', 'mobile_number': '+4407700900460', 'password': 'validPassword!', - 'email_address': session['invited_org_user']['email_address'], - 'organisation': session['invited_org_user']['organisation'] + 'email_address': sample_org_invite['email_address'], + 'organisation': sample_org_invite['organisation'] }) assert response.status_code == 302 assert response.location == url_for('main.verify', _external=True) mock_get_user_by_email.assert_called_once_with( - session['invited_org_user']['email_address'] + sample_org_invite['email_address'] ) assert mock_register_user.called is False assert mock_send_already_registered_email.called is False @@ -310,47 +311,6 @@ def test_org_user_registration( mock_send_verify_email, mock_accept_org_invite, mock_add_user_to_organisation, -): - invited_org_user = InvitedOrgUser(sample_org_invite) - with client.session_transaction() as session: - session['invited_org_user'] = invited_org_user.serialize() - - response = client.post(url_for('main.register_from_org_invite'), data={ - 'name': 'Test User', - 'email_address': session['invited_org_user']['email_address'], - 'mobile_number': '+4407700900460', - 'password': 'validPassword!', - 'organisation': session['invited_org_user']['organisation'] - }) - - assert response.status_code == 302 - assert response.location == url_for('main.verify', _external=True) - - assert mock_get_user_by_email.called is False - mock_register_user.assert_called_once_with( - 'Test User', - session['invited_org_user']['email_address'], - '+4407700900460', - 'validPassword!', - 'sms_auth' - ) - mock_send_verify_code.assert_called_once_with( - '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6', - 'sms', - '+4407700900460', - ) - - -def test_org_user_registration_when_org_user_id_in_session( - client, - sample_org_invite, - mock_email_is_not_already_in_use, - mock_register_user, - mock_send_verify_code, - mock_get_user_by_email, - mock_send_verify_email, - mock_accept_org_invite, - mock_add_user_to_organisation, mock_get_invited_org_user_by_id, ): with client.session_transaction() as session: @@ -368,7 +328,6 @@ def test_org_user_registration_when_org_user_id_in_session( assert response.location == url_for('main.verify', _external=True) assert mock_get_user_by_email.called is False - mock_get_invited_org_user_by_id.assert_called_once_with(sample_org_invite['id']) mock_register_user.assert_called_once_with( 'Test User', sample_org_invite['email_address'], @@ -381,6 +340,7 @@ def test_org_user_registration_when_org_user_id_in_session( 'sms', '+4407700900460', ) + mock_get_invited_org_user_by_id.assert_called_once_with(sample_org_invite['id']) def test_verified_org_user_redirects_to_dashboard( diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index cb36cf8d0..3c6c100fc 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -133,7 +133,7 @@ def test_invite_goes_in_session( ) with client_request.session_transaction() as session: - assert session['invited_user']['email_address'] == 'test@user.gov.uk' + assert session['invited_user_id'] == sample_invite['id'] @pytest.mark.parametrize('user, landing_page_title', [ @@ -410,25 +410,18 @@ def test_new_user_accept_invite_completes_new_registration_redirects_to_verify( mock_get_service, mocker, ): - expected_service = service_one['id'] - expected_email = sample_invite['email_address'] - expected_from_user = service_one['users'][0] expected_redirect_location = 'http://localhost/register-from-invite' response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) with client.session_transaction() as session: assert response.status_code == 302 assert response.location == expected_redirect_location - invited_user = session.get('invited_user') - assert invited_user + assert 'invited_user' not in session assert session.get('invited_user_id') == USER_ONE_ID - assert expected_service == invited_user['service'] - assert expected_email == invited_user['email_address'] - assert expected_from_user == invited_user['from_user'] - data = {'service': invited_user['service'], - 'email_address': invited_user['email_address'], - 'from_user': invited_user['from_user'], + data = {'service': sample_invite['service'], + 'email_address': sample_invite['email_address'], + 'from_user': sample_invite['from_user'], 'password': 'longpassword', 'mobile_number': '+447890123456', 'name': 'Invited User', diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 2e55b24c3..81d83f802 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -1,4 +1,3 @@ -from datetime import datetime from unittest.mock import ANY import pytest @@ -6,7 +5,6 @@ from bs4 import BeautifulSoup from flask import session, url_for from flask_login import current_user -from app.models.user import InvitedUser from tests.conftest import normalize_spaces @@ -208,19 +206,12 @@ def test_shows_name_on_registration_page_from_invite( fake_uuid, email_address, expected_value, + sample_invite, + mock_get_invited_user_by_id, ): + sample_invite['email_address'] = email_address with client_request.session_transaction() as session: - session['invited_user'] = { - 'id': fake_uuid, - 'service': fake_uuid, - 'from_user': "", - 'email_address': email_address, - 'permissions': ["manage_users"], - 'status': "pending", - 'created_at': datetime.utcnow(), - 'auth_type': 'sms_auth', - 'folder_permissions': [], - } + session['invited_user_id'] = sample_invite page = client_request.get('main.register_from_invite') assert page.select_one('input[name=name]').get('value') == expected_value @@ -229,30 +220,23 @@ def test_shows_name_on_registration_page_from_invite( def test_shows_hidden_email_address_on_registration_page_from_invite( client_request, fake_uuid, + sample_invite, + mock_get_invited_user_by_id, ): + with client_request.session_transaction() as session: - session['invited_user'] = { - 'id': fake_uuid, - 'service': fake_uuid, - 'from_user': "", - 'email_address': "test@example.com", - 'permissions': ["manage_users"], - 'status': "pending", - 'created_at': datetime.utcnow(), - 'auth_type': 'sms_auth', - 'folder_permissions': [], - } + session['invited_user_id'] = sample_invite page = client_request.get('main.register_from_invite') assert normalize_spaces(page.select_one('main p').text) == ( - 'Your account will be created with this email address: test@example.com' + 'Your account will be created with this email address: invited_user@test.gov.uk' ) hidden_input = page.select_one('form .govuk-visually-hidden input') for attr, value in ( ('type', 'email'), ('name', 'username'), ('id', 'username'), - ('value', 'test@example.com'), + ('value', 'invited_user@test.gov.uk'), ('disabled', "disabled"), ('tabindex', '-1'), ('aria-hidden', 'true'), @@ -275,30 +259,19 @@ def test_register_from_invite( mock_register_user, mock_send_verify_code, mock_accept_invite, + mock_get_invited_user_by_id, + sample_invite, extra_data, ): - invited_user = InvitedUser( - { - 'id': fake_uuid, - 'service': fake_uuid, - 'from_user': "", - 'email_address': "invited@user.com", - 'permissions': ["manage_users"], - 'status': "pending", - 'created_at': datetime.utcnow(), - 'auth_type': 'sms_auth', - 'folder_permissions': [], - } - ) with client.session_transaction() as session: - session['invited_user'] = invited_user.serialize() + session['invited_user_id'] = sample_invite['id'] response = client.post( url_for('main.register_from_invite'), data=dict( name='Registered in another Browser', - email_address=invited_user.email_address, + email_address=sample_invite['email_address'], mobile_number='+4407700900460', - service=str(invited_user.id), + service=sample_invite['service'], password='somreallyhardthingtoguess', auth_type='sms_auth', **extra_data @@ -308,11 +281,12 @@ def test_register_from_invite( assert response.location == url_for('main.verify', _external=True) mock_register_user.assert_called_once_with( 'Registered in another Browser', - invited_user.email_address, + sample_invite['email_address'], '+4407700900460', 'somreallyhardthingtoguess', 'sms_auth', - ) + ), + mock_get_invited_user_by_id.assert_called_once_with(sample_invite['id']) def test_register_from_invite_when_user_registers_in_another_browser( @@ -320,29 +294,19 @@ def test_register_from_invite_when_user_registers_in_another_browser( api_user_active, mock_get_user_by_email, mock_accept_invite, + mock_get_invited_user_by_id, + sample_invite, ): - invited_user = InvitedUser( - { - 'id': api_user_active['id'], - 'service': api_user_active['id'], - 'from_user': "", - 'email_address': api_user_active['email_address'], - 'permissions': ["manage_users"], - 'status': "pending", - 'created_at': datetime.utcnow(), - 'auth_type': 'sms_auth', - 'folder_permissions': [], - } - ) + sample_invite['email_address'] = api_user_active['email_address'] with client.session_transaction() as session: - session['invited_user'] = invited_user.serialize() + session['invited_user_id'] = sample_invite['id'] response = client.post( url_for('main.register_from_invite'), data={ 'name': 'Registered in another Browser', 'email_address': api_user_active['email_address'], 'mobile_number': api_user_active['mobile_number'], - 'service': str(api_user_active['id']), + 'service': sample_invite['service'], 'password': 'somreallyhardthingtoguess', 'auth_type': 'sms_auth' } @@ -364,6 +328,7 @@ def test_register_from_email_auth_invite( mock_create_event, mock_add_user_to_service, mock_get_service, + mock_get_invited_user_by_id, invite_email_address, service_one, fake_uuid, @@ -374,7 +339,7 @@ def test_register_from_email_auth_invite( sample_invite['auth_type'] = 'email_auth' sample_invite['email_address'] = invite_email_address with client.session_transaction() as session: - session['invited_user'] = sample_invite + session['invited_user_id'] = sample_invite['id'] assert not current_user.is_authenticated data = { @@ -401,6 +366,8 @@ def test_register_from_email_auth_invite( data['password'], data['auth_type'] ) + # this is actually called twice, at the beginning of the function and then by the activate_user function + mock_get_invited_user_by_id.assert_called_with(sample_invite['id']) mock_accept_invite.assert_called_once_with(sample_invite['service'], sample_invite['id']) # just logs them in assert current_user.is_authenticated @@ -412,7 +379,7 @@ def test_register_from_email_auth_invite( with client.session_transaction() as session: # invited user details are still there so they can get added to the service - assert session['invited_user'] == sample_invite + assert session['invited_user_id'] == sample_invite['id'] def test_can_register_email_auth_without_phone_number( @@ -427,10 +394,11 @@ def test_can_register_email_auth_without_phone_number( mock_create_event, mock_add_user_to_service, mock_get_service, + mock_get_invited_user_by_id, ): sample_invite['auth_type'] = 'email_auth' with client.session_transaction() as session: - session['invited_user'] = sample_invite + session['invited_user_id'] = sample_invite['id'] data = { 'name': 'invited user', @@ -474,11 +442,12 @@ def test_cannot_register_with_sms_auth_and_missing_mobile_number( def test_register_from_invite_form_doesnt_show_mobile_number_field_if_email_auth( client, - sample_invite + sample_invite, + mock_get_invited_user_by_id, ): sample_invite['auth_type'] = 'email_auth' with client.session_transaction() as session: - session['invited_user'] = sample_invite + session['invited_user_id'] = sample_invite['id'] response = client.get(url_for('main.register_from_invite')) diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index 29a7d4d05..9d4aafbcf 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -224,7 +224,8 @@ def test_email_address_is_treated_case_insensitively_when_signing_in_as_invited_ api_user_active, sample_invite, mock_accept_invite, - mock_send_verify_code + mock_send_verify_code, + mock_get_invited_user_by_id, ): sample_invite['email_address'] = 'TEST@user.gov.uk' @@ -234,7 +235,7 @@ def test_email_address_is_treated_case_insensitively_when_signing_in_as_invited_ ) with client.session_transaction() as session: - session['invited_user'] = sample_invite + session['invited_user_id'] = sample_invite['id'] response = client.post( url_for('main.sign_in'), data={ @@ -244,3 +245,4 @@ def test_email_address_is_treated_case_insensitively_when_signing_in_as_invited_ assert mock_accept_invite.called assert response.status_code == 302 assert mock_send_verify_code.called + mock_get_invited_user_by_id.assert_called_once_with(sample_invite['id']) diff --git a/tests/app/main/views/test_verify.py b/tests/app/main/views/test_verify.py index 709164d34..98107e3b6 100644 --- a/tests/app/main/views/test_verify.py +++ b/tests/app/main/views/test_verify.py @@ -176,6 +176,7 @@ def test_activate_user_redirects_to_service_dashboard_if_user_already_belongs_to api_user_active, mock_login, mock_get_service, + mock_get_invited_user_by_id, ): mocker.patch('app.user_api_client.add_user_to_service', side_effect=HTTPError( response=Mock( @@ -189,10 +190,10 @@ def test_activate_user_redirects_to_service_dashboard_if_user_already_belongs_to )) # Can't use `with client.session_transaction()...` here since activate_session is not a view function - flask_session['invited_user'] = sample_invite + flask_session['invited_user_id'] = sample_invite['id'] response = activate_user(api_user_active['id']) assert response.location == url_for('main.service_dashboard', service_id=service_one['id']) - flask_session.pop('invited_user') + flask_session.pop('invited_user_id') diff --git a/tests/app/models/test_user.py b/tests/app/models/test_user.py index 8bc2d24ea..1d0e044a5 100644 --- a/tests/app/models/test_user.py +++ b/tests/app/models/test_user.py @@ -137,13 +137,6 @@ def test_invited_user_from_session_uses_id_even_if_obj_in_session( mock_get_invited_user_by_id.assert_called_once_with(USER_ONE_ID) -def test_invited_user_from_session_uses_obj_if_id_not_present(client, mocker, sample_invite): - session_dict = {'invited_user': sample_invite} - mocker.patch.dict('app.models.user.session', values=session_dict, clear=True) - - assert InvitedUser.from_session().id == USER_ONE_ID - - def test_invited_user_from_session_returns_none_if_nothing_present(client, mocker): mocker.patch.dict('app.models.user.session', values={}, clear=True) assert InvitedUser.from_session() is None @@ -176,13 +169,6 @@ def test_invited_org_user_from_session_uses_id_even_if_obj_in_session( mock_get_invited_org_user_by_id.assert_called_once_with(fake_id) -def test_invited_org_user_from_session_uses_obj_if_id_not_present(client, mocker, sample_org_invite): - session_dict = {'invited_org_user': sample_org_invite} - mocker.patch.dict('app.models.user.session', values=session_dict, clear=True) - - assert InvitedOrgUser.from_session().id == sample_org_invite['id'] - - def test_invited_org_user_from_session_returns_none_if_nothing_present(client, mocker): mocker.patch.dict('app.models.user.session', values={}, clear=True) assert InvitedOrgUser.from_session() is None