From bd9acb131077745cd2c40ce82c13467a887d1b8a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 5 Jun 2019 10:47:55 +0100 Subject: [PATCH] Make invited user model inherit from JSONModel This is more consistent, and less fiddly that always having to call it with a dictionary expansion. --- app/main/views/verify.py | 2 +- app/models/__init__.py | 1 + app/models/user.py | 90 ++++++++++--------- .../test_organisation_invites.py | 10 +-- tests/app/main/views/test_register.py | 60 ++++++++----- tests/app/main/views/test_sign_in.py | 2 +- tests/conftest.py | 49 ++++++---- 7 files changed, 127 insertions(+), 87 deletions(-) diff --git a/app/main/views/verify.py b/app/main/views/verify.py index fcd2bec48..d9f2e331e 100644 --- a/app/main/views/verify.py +++ b/app/main/views/verify.py @@ -90,7 +90,7 @@ def activate_user(user_id): def _add_invited_user_to_service(invited_user): - invitation = InvitedUser(**invited_user) + invitation = InvitedUser(invited_user) user = User.from_id(session['user_id']) service_id = invited_user['service'] user_api_client.add_user_to_service(service_id, user.id, invitation.permissions, invitation.folder_permissions) diff --git a/app/models/__init__.py b/app/models/__init__.py index 0b2fedae5..ec7de50b0 100644 --- a/app/models/__init__.py +++ b/app/models/__init__.py @@ -18,6 +18,7 @@ class JSONModel(): def __getattr__(self, attr): if attr in self.ALLOWED_PROPERTIES: return self._dict[attr] + return raise AttributeError('`{}` is not a {} attribute'.format( attr, self.__class__.__name__.lower(), diff --git a/app/models/user.py b/app/models/user.py index 26e8bc687..764f13470 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -315,34 +315,24 @@ class User(JSONModel, UserMixin): session['current_session_id'] = self.current_session_id -class InvitedUser(object): +class InvitedUser(JSONModel): - def __init__(self, - id, - service, - from_user, - email_address, - permissions, - status, - created_at, - auth_type, - folder_permissions): - self.id = id - self.service = str(service) - self._from_user = from_user - self.email_address = email_address - if isinstance(permissions, list): - self.permissions = permissions - else: - if permissions: - self.permissions = permissions.split(',') - else: - self.permissions = [] - self.status = status - self.created_at = created_at - self.auth_type = auth_type - self.permissions = translate_permissions_from_db_to_admin_roles(self.permissions) - self.folder_permissions = folder_permissions + ALLOWED_PROPERTIES = { + 'id', + 'service', + 'from_user', + 'email_address', + 'permissions', + 'status', + 'created_at', + 'auth_type', + 'folder_permissions', + } + + def __init__(self, _dict): + super().__init__(_dict) + self.permissions = _dict.get('permissions') or [] + self._from_user = _dict['from_user'] @classmethod def create( @@ -354,7 +344,7 @@ class InvitedUser(object): auth_type, folder_permissions, ): - return cls(**invite_api_client.create_invite( + return cls(invite_api_client.create_invite( invite_from_id, service_id, email_address, @@ -374,6 +364,18 @@ class InvitedUser(object): self.folder_permissions, ) + @property + def permissions(self): + return self._permissions + + @permissions.setter + def permissions(self, permissions): + if isinstance(permissions, list): + self._permissions = permissions + else: + self._permissions = permissions.split(',') + self._permissions = translate_permissions_from_db_to_admin_roles(self.permissions) + @property def from_user(self): return User.from_id(self._from_user) @@ -389,7 +391,7 @@ class InvitedUser(object): @classmethod def from_token(cls, token): try: - return cls(**invite_api_client.check_token(token)) + return cls(invite_api_client.check_token(token)) except HTTPError as exception: if exception.status_code == 400 and 'invitation' in exception.message: raise InviteTokenError(exception.message['invitation']) @@ -399,7 +401,7 @@ class InvitedUser(object): @classmethod def from_session(cls): invited_user = session.get('invited_user') - return cls(**invited_user) if invited_user else None + return cls(invited_user) if invited_user else None def has_permissions(self, *permissions): if self.status == 'cancelled': @@ -445,15 +447,19 @@ class InvitedUser(object): return [{'id': x} for x in self.folder_permissions] -class InvitedOrgUser(object): +class InvitedOrgUser(JSONModel): - def __init__(self, id, organisation, invited_by, email_address, status, created_at): - self.id = id - self.organisation = str(organisation) - self._invited_by = invited_by - self.email_address = email_address - self.status = status - self.created_at = created_at + ALLOWED_PROPERTIES = { + 'id', + 'organisation', + 'email_address', + 'status', + 'created_at', + } + + def __init__(self, _dict): + super().__init__(_dict) + self._invited_by = _dict['invited_by'] def __eq__(self, other): return ((self.id, @@ -468,14 +474,14 @@ class InvitedOrgUser(object): @classmethod def create(cls, invite_from_id, org_id, email_address): - return cls(**org_invite_api_client.create_invite( + return cls(org_invite_api_client.create_invite( invite_from_id, org_id, email_address )) @classmethod def from_session(cls): invited_org_user = session.get('invited_org_user') - return cls(**invited_org_user) if invited_org_user else None + return cls(invited_org_user) if invited_org_user else None def serialize(self, permissions_as_string=False): data = {'id': self.id, @@ -494,7 +500,7 @@ class InvitedOrgUser(object): @classmethod def from_token(cls, token): try: - return cls(**org_invite_api_client.check_token(token)) + return cls(org_invite_api_client.check_token(token)) except HTTPError as exception: if exception.status_code == 400 and 'invitation' in exception.message: raise InviteTokenError(exception.message['invitation']) @@ -552,7 +558,7 @@ class InvitedUsers(Users): ] def __getitem__(self, index): - return self.model(**self.users[index]) + return self.model(self.users[index]) class OrganisationInvitedUsers(InvitedUsers): diff --git a/tests/app/main/views/organisations/test_organisation_invites.py b/tests/app/main/views/organisations/test_organisation_invites.py index 8b0492284..7d623dbfa 100644 --- a/tests/app/main/views/organisations/test_organisation_invites.py +++ b/tests/app/main/views/organisations/test_organisation_invites.py @@ -231,7 +231,7 @@ def test_registration_from_org_invite_has_bad_data( data, error ): - invited_org_user = InvitedOrgUser(**sample_org_invite) + invited_org_user = InvitedOrgUser(sample_org_invite) with client.session_transaction() as session: session['invited_org_user'] = invited_org_user.serialize() @@ -251,7 +251,7 @@ def test_registration_from_org_invite_has_different_email_or_organisation( sample_org_invite, diff_data ): - invited_org_user = InvitedOrgUser(**sample_org_invite) + invited_org_user = InvitedOrgUser(sample_org_invite) with client.session_transaction() as session: session['invited_org_user'] = invited_org_user.serialize() @@ -278,7 +278,7 @@ def test_org_user_registers_with_email_already_in_use( mock_send_already_registered_email, mock_register_user ): - invited_org_user = InvitedOrgUser(**sample_org_invite) + invited_org_user = InvitedOrgUser(sample_org_invite) with client.session_transaction() as session: session['invited_org_user'] = invited_org_user.serialize() @@ -311,7 +311,7 @@ def test_org_user_registration( mock_accept_org_invite, mock_add_user_to_organisation, ): - invited_org_user = InvitedOrgUser(**sample_org_invite) + invited_org_user = InvitedOrgUser(sample_org_invite) with client.session_transaction() as session: session['invited_org_user'] = invited_org_user.serialize() @@ -349,7 +349,7 @@ def test_verified_org_user_redirects_to_dashboard( mock_activate_user, mock_login, ): - invited_org_user = InvitedOrgUser(**sample_org_invite).serialize() + invited_org_user = InvitedOrgUser(sample_org_invite).serialize() with client.session_transaction() as session: session['expiry_date'] = str(datetime.utcnow() + timedelta(hours=1)) session['user_details'] = {"email": invited_org_user['email_address'], "id": invited_org_user['id']} diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index b8e7f143b..e9d0aa6dc 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -199,14 +199,18 @@ def test_shows_registration_page_from_invite( expected_value, ): with client_request.session_transaction() as session: - session['invited_user'] = InvitedUser( - fake_uuid, fake_uuid, "", - email_address, - ["manage_users"], - "pending", - datetime.utcnow(), - 'sms_auth', [] - ).serialize() + 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': [], + } + page = client_request.get('main.register_from_invite') assert page.select_one('input[name=name]')['value'] == expected_value @@ -219,13 +223,19 @@ def test_register_from_invite( mock_send_verify_code, mock_accept_invite, ): - invited_user = InvitedUser(fake_uuid, fake_uuid, "", - "invited@user.com", - ["manage_users"], - "pending", - datetime.utcnow(), - 'sms_auth', - []) + 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() response = client.post( @@ -256,13 +266,19 @@ def test_register_from_invite_when_user_registers_in_another_browser( mock_get_user_by_email, mock_accept_invite, ): - invited_user = InvitedUser(api_user_active['id'], api_user_active['id'], "", - api_user_active['email_address'], - ["manage_users"], - "pending", - datetime.utcnow(), - 'sms_auth', - []) + 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': [], + } + ) with client.session_transaction() as session: session['invited_user'] = invited_user.serialize() response = client.post( diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index f4525286d..d4b9b2d71 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -45,7 +45,7 @@ def test_doesnt_redirect_to_sign_in_if_no_session_info( api_user_active, mock_get_organisation_by_domain, ): - assert 'current_session_id' not in api_user_active + api_user_active['current_session_id'] = str(uuid.UUID(int=1)) with client_request.session_transaction() as session: session['current_session_id'] = None diff --git a/tests/conftest.py b/tests/conftest.py index b2f05d0ab..0f6bcffa1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1088,7 +1088,8 @@ def api_user_pending(fake_uuid): 'state': 'pending', 'failed_login_count': 0, 'permissions': {}, - 'organisations': [] + 'organisations': [], + 'current_session_id': None, } return user_data @@ -1112,7 +1113,8 @@ def platform_admin_user(fake_uuid): 'view_activity']}, 'platform_admin': True, 'auth_type': 'sms_auth', - 'organisations': [] + 'organisations': [], + 'current_session_id': None, } return user_data @@ -1130,7 +1132,8 @@ def api_user_active(fake_uuid, email_address='test@user.gov.uk'): 'platform_admin': False, 'auth_type': 'sms_auth', 'password_changed_at': str(datetime.utcnow()), - 'organisations': [] + 'organisations': [], + 'current_session_id': None, } return user_data @@ -1148,7 +1151,8 @@ def api_user_active_email_auth(fake_uuid, email_address='test@user.gov.uk'): 'platform_admin': False, 'auth_type': 'email_auth', 'password_changed_at': str(datetime.utcnow()), - 'organisations': [] + 'organisations': [], + 'current_session_id': None, } return user_data @@ -1176,7 +1180,8 @@ def api_nongov_user_active(fake_uuid): 'platform_admin': False, 'auth_type': 'sms_auth', 'password_changed_at': str(datetime.utcnow()), - 'organisations': [] + 'organisations': [], + 'current_session_id': None, } return user_data @@ -1202,7 +1207,8 @@ def active_user_with_permissions(fake_uuid): 'platform_admin': False, 'auth_type': 'sms_auth', 'organisations': [ORGANISATION_ID], - 'services': [SERVICE_ONE_ID] + 'services': [SERVICE_ONE_ID], + 'current_session_id': None, } return user_data @@ -1238,6 +1244,7 @@ def active_user_with_permission_to_two_services(fake_uuid): 'auth_type': 'sms_auth', 'organisations': [ORGANISATION_ID], 'services': [SERVICE_ONE_ID, SERVICE_TWO_ID], + 'current_session_id': None, } @@ -1261,7 +1268,8 @@ def active_caseworking_user(fake_uuid): 'platform_admin': False, 'auth_type': 'sms_auth', 'organisations': [], - 'services': [SERVICE_ONE_ID] + 'services': [SERVICE_ONE_ID], + 'current_session_id': None, } return user_data @@ -1287,7 +1295,8 @@ def active_user_no_mobile(fake_uuid): 'platform_admin': False, 'auth_type': 'email_auth', 'organisations': [], - 'services': [SERVICE_ONE_ID] + 'services': [SERVICE_ONE_ID], + 'current_session_id': None, } return user_data @@ -1306,7 +1315,8 @@ def active_user_view_permissions(fake_uuid): 'platform_admin': False, 'auth_type': 'sms_auth', 'organisations': [], - 'services': [SERVICE_ONE_ID] + 'services': [SERVICE_ONE_ID], + 'current_session_id': None, } return user_data @@ -1325,7 +1335,8 @@ def active_user_empty_permissions(fake_uuid): 'platform_admin': False, 'auth_type': 'sms_auth', 'organisations': [], - 'services': [SERVICE_ONE_ID] + 'services': [SERVICE_ONE_ID], + 'current_session_id': None, } return user_data @@ -1348,7 +1359,8 @@ def active_user_manage_template_permission(fake_uuid): 'platform_admin': False, 'auth_type': 'sms_auth', 'organisations': [], - 'services': [SERVICE_ONE_ID] + 'services': [SERVICE_ONE_ID], + 'current_session_id': None, } @@ -1370,7 +1382,8 @@ def active_user_no_api_key_permission(fake_uuid): ]}, 'platform_admin': False, 'auth_type': 'sms_auth', - 'organisations': [] + 'organisations': [], + 'current_session_id': None, } @@ -1391,7 +1404,8 @@ def active_user_no_settings_permission(fake_uuid): 'view_activity', ]}, 'platform_admin': False, - 'auth_type': 'sms_auth' + 'auth_type': 'sms_auth', + 'current_session_id': None, } @@ -1406,7 +1420,8 @@ def api_user_locked(fake_uuid): 'failed_login_count': 5, 'permissions': {}, 'auth_type': 'sms_auth', - 'organisations': [] + 'organisations': [], + 'current_session_id': None, } return user_data @@ -1423,7 +1438,8 @@ def api_user_request_password_reset(fake_uuid): 'permissions': {}, 'password_changed_at': None, 'auth_type': 'sms_auth', - 'organisations': [] + 'organisations': [], + 'current_session_id': None, } return user_data @@ -1440,7 +1456,8 @@ def api_user_changed_password(fake_uuid): 'permissions': {}, 'auth_type': 'sms_auth', 'password_changed_at': str(datetime.utcnow() + timedelta(minutes=1)), - 'organisations': [] + 'organisations': [], + 'current_session_id': None, } return user_data