Merge pull request #3841 from alphagov/remove-old-invite-stuff

stop putting invite user objects in the session

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)

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) if invited_user_id else None
-            return cls.by_id(invited_user_id)
-
-        invited_user = session.get('invited_user')
-        return cls(invited_user) if invited_user 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) if invited_org_user_id else None
-            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
 
     @classmethod
     def by_id_and_org_id(cls, org_id, invited_user_id):

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:
+    data = {
-        session['invited_org_user'][data] = 'different'
-
-    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'],
+        'email_address': sample_org_invite['email_address'],
-        'organisation': session['invited_org_user']['organisation']
+        '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'],
+        'email_address': sample_org_invite['email_address'],
-        'organisation': session['invited_org_user']['organisation']
+        '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(

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' not in session
-        assert invited_user
         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'],
+    data = {'service': sample_invite['service'],
-            'email_address': invited_user['email_address'],
+            'email_address': sample_invite['email_address'],
-            'from_user': invited_user['from_user'],
+            'from_user': sample_invite['from_user'],
             'password': 'longpassword',
             'mobile_number': '+447890123456',
             'name': 'Invited User',

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'] = {
+        session['invited_user_id'] = sample_invite
-            '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': [],
-        }
 
     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'] = {
+        session['invited_user_id'] = sample_invite
-            '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': [],
-        }
 
     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(
+    sample_invite['email_address'] = api_user_active['email_address']
-        {
-            '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': [],
-        }
-    )
     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'))
 

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'])

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')

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