darkhelm/notifications-admin
1
0
Fork 0
mirror of https://github.com/GSA/notifications-admin.git synced 2026-08-14 23:08:07 -04:00
Code Issues Packages Projects Releases Wiki Activity
Files
03321da575ea1733cbbe6a85eb4e5cd1cada183d
notifications-admin/app/main/views/invites.py

118 lines
5.0 KiB
Python
Raw Normal View History

Enforce order and style of imports Done using isort[1], with the following command: ``` isort -rc ./app ./tests ``` Adds linting to the `run_tests.sh` script to stop badly-sorted imports getting re-introduced. Chosen style is ‘Vertical Hanging Indent’ with trailing commas, because I think it gives the cleanest diffs, eg: ``` from third_party import ( lib1, lib2, lib3, lib4, ) ``` 1. https://pypi.python.org/pypi/isort
2018-02-20 11:22:17 +00:00
from flask import abort, flash, redirect, render_template, session, url_for
make sure invite tokens still check token on admin for error handler to kick in
2017-11-01 16:02:05 +00:00
from flask_login import current_user
Enforce order and style of imports Done using isort[1], with the following command: ``` isort -rc ./app ./tests ``` Adds linting to the `run_tests.sh` script to stop badly-sorted imports getting re-introduced. Chosen style is ‘Vertical Hanging Indent’ with trailing commas, because I think it gives the cleanest diffs, eg: ``` from third_party import ( lib1, lib2, lib3, lib4, ) ``` 1. https://pypi.python.org/pypi/isort
2018-02-20 11:22:17 +00:00
from markupsafe import Markup
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
Enforce order and style of imports Done using isort[1], with the following command: ``` isort -rc ./app ./tests ``` Adds linting to the `run_tests.sh` script to stop badly-sorted imports getting re-introduced. Chosen style is ‘Vertical Hanging Indent’ with trailing commas, because I think it gives the cleanest diffs, eg: ``` from third_party import ( lib1, lib2, lib3, lib4, ) ``` 1. https://pypi.python.org/pypi/isort
2018-02-20 11:22:17 +00:00
from app.main import main
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
from app.models.organisation import Organisation
from app.models.service import Service
from app.models.user import (
InvitedOrgUser,
InvitedUser,
OrganisationUsers,
User,
Users,
)
[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
2016-02-29 17:35:21 +00:00
@main.route("/invitation/<token>")
def accept_invite(token):
move invite error handler to top level ensure we catch org errors as well as regular errors
2020-03-06 11:53:55 +00:00
invited_user = InvitedUser.from_token(token)
Merge branch 'master' into error-handling Conflicts: app/main/views/invites.py
2016-03-10 15:03:21 +00:00
Make email addresses case insensitive when inviting users to services Email addresses in invites should be case insensitive. This is to stop the bug where a user creates their account using a lower case email address (e.g. user1@gov.uk), but is then invited to a service using their email address in a different case (e.g. USER1.gov.uk) and sees an error message telling them that they can't accept an invite for a different email address.
2017-12-21 16:42:16 +00:00
if not current_user.is_anonymous and current_user.email_address.lower() != invited_user.email_address.lower():
Wrap message in Markup
2016-04-26 12:12:47 +01:00
message = Markup("""
More helpful error when signed in an accept invite
2016-04-07 15:03:36 +01:00
You’re signed in as {}.
This invite is for another email address.
Don’t allow unstyled links They should always be styled with the `govuk-link` class from GOV.UK Frontend, or another custom class.
2020-05-29 17:25:11 +01:00
<a href={} class="govuk-link govuk-link--no-visited-state">Sign out</a>
and click the link again to accept this invite.
Wrap message in Markup
2016-04-26 12:12:47 +01:00
""".format(
More helpful error when signed in an accept invite
2016-04-07 15:03:36 +01:00
current_user.email_address,
Stop using `_external=True` in tests It looks like, by default, Flask no longer makes full URLs, for example `https://example.com/path`. Instead it does `/path`. This will still work fine, and if anything is better because it reduces the number of bytes of HTML we are sending. It won’t mean that requests go over `http` instead of `https` without the protocol because we set the appropriate HSTS header here: https://github.com/alphagov/notifications-aws/blob/0c57da7781260c56575b796062795a9d8e1f8c87/ansible/roles/paas-proxy/templates/admin.conf.j2#L11 This commit changes all our tests to reflect that URLs no longer have the protocol and domain in them. `_external=True` is Flask’s way of saying whether a URL should be generated with the domain and protocol (`True`) or without it (`False`). Again, I can’t find the changelog or diff where this was introuduced, but if you’d like to go spelunking then here’s a starting point: https://github.com/pallets/flask/blob/50374e3cfe815acd24fd390c861561a0dd17bfbb/src/flask/helpers.py#L192
2022-05-27 14:46:35 +01:00
url_for("main.sign_out")))
Wrap message in Markup
2016-04-26 12:12:47 +01:00
flash(message=message)
Removed some un needed flash messages raised as bugs. In the process found a couple of edge cases of incorrect use of invitation links by other users which are now handled.
2016-03-30 16:16:34 +01:00
abort(403)
Refactored register_errorhandlers so that it handles HTTPError Remove most cases where we catch HTTPError
2016-03-10 11:53:29 +00:00
if invited_user.status == 'cancelled':
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
service = Service.from_id(invited_user.service)
Refactored register_errorhandlers so that it handles HTTPError Remove most cases where we catch HTTPError
2016-03-10 11:53:29 +00:00
return render_template('views/cancelled-invitation.html',
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
from_user=invited_user.from_user.name,
service_name=service.name)
Merge branch 'master' into error-handling Conflicts: app/main/views/invites.py
2016-03-10 15:03:21 +00:00
if invited_user.status == 'accepted':
store invited user ids in session same as the invited org user ids in the previous commit
2021-03-08 12:51:35 +00:00
session.pop('invited_user_id', None)
Add a tour for users new to broadcast services This is an initial, prototype-quality attempt at introducing some kind of tour for users new to broadcasting. A lot of the users we’re speaking to don’t have a good concept of what broadcasting means, which is causing usability problems down the line. We did a similar thing in the early days of Notify to explain the concept of message templates and personalisation.
2020-08-03 13:37:34 +01:00
service = Service.from_id(invited_user.service)
Merge branch 'master' into error-handling Conflicts: app/main/views/invites.py
2016-03-10 15:03:21 +00:00
return redirect(url_for('main.service_dashboard', service_id=invited_user.service))
store invited user ids in session same as the invited org user ids in the previous commit
2021-03-08 12:51:35 +00:00
session['invited_user_id'] = invited_user.id
Refactored register_errorhandlers so that it handles HTTPError Remove most cases where we catch HTTPError
2016-03-10 11:53:29 +00:00
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
existing_user = User.from_email_address_or_none(invited_user.email_address)
Additional check needed to see if user was already a user for the service that they were invited to.
2016-03-15 15:32:30 +00:00
if existing_user:
Update `email_access_validated_at` on invite Accepting an invite means that you’ve just clicked a link in your email inbox. This shows that you have access to your email. We can make a record of this, thereby extending the time before we ask you to revalidate your email address.
2021-08-17 16:38:37 +01:00
existing_user.update_email_access_validated_at()
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
invited_user.accept_invite()
if existing_user in Users(invited_user.service):
Additional check needed to see if user was already a user for the service that they were invited to.
2016-03-15 15:32:30 +00:00
return redirect(url_for('main.service_dashboard', service_id=invited_user.service))
else:
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
service = Service.from_id(invited_user.service)
Prevent switching auth type for Platform Admins This closes a security loophole, where the auth type of a Platform Admin could be unwittingly changed when they accept an invite, or by an admin of a service they are a member of.
2021-05-25 15:58:33 +01:00
# if the service you're being added to can modify auth type, then check if we can do this;
# if the user is a Platform Admin, we silently leave this unchanged to prevent a security
# issue where someone could switch their auth type to something less secure
if service.has_permission('email_auth') and not existing_user.platform_admin:
if invited_user.auth_type == 'email_auth' or (
# they have a phone number, we want them to start using it.
# if they dont have a mobile we just ignore that option of the invite
existing_user.mobile_number and invited_user.auth_type == 'sms_auth'
):
existing_user.update(auth_type=invited_user.auth_type)
Refactor model to put `add_to…` methods on user An invited user can’t be added to an organisation or service, only a real user can. So the methods to do this should be on the user model, and take the details of the invite as arguments.
2019-06-27 15:48:29 +01:00
existing_user.add_to_service(
service_id=invited_user.service,
permissions=invited_user.permissions,
folder_permissions=invited_user.folder_permissions,
Add audit event for inviting users to a service
2021-03-05 12:19:36 +00:00
invited_by_id=invited_user.from_user.id,
Refactor model to put `add_to…` methods on user An invited user can’t be added to an organisation or service, only a real user can. So the methods to do this should be on the user model, and take the details of the invite as arguments.
2019-06-27 15:48:29 +01:00
)
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
return redirect(url_for('main.service_dashboard', service_id=service.id))
Refactored register_errorhandlers so that it handles HTTPError Remove most cases where we catch HTTPError
2016-03-10 11:53:29 +00:00
else:
return redirect(url_for('main.register_from_invite'))
invite-team-members
2018-02-19 16:53:29 +00:00
@main.route("/organisation-invitation/<token>")
def accept_org_invite(token):
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
invited_org_user = InvitedOrgUser.from_token(token)
move invite error handler to top level ensure we catch org errors as well as regular errors
2020-03-06 11:53:55 +00:00
invite-team-members
2018-02-19 16:53:29 +00:00
if not current_user.is_anonymous and current_user.email_address.lower() != invited_org_user.email_address.lower():
message = Markup("""
You’re signed in as {}.
This invite is for another email address.
Don’t allow unstyled links They should always be styled with the `govuk-link` class from GOV.UK Frontend, or another custom class.
2020-05-29 17:25:11 +01:00
<a class="govuk-link govuk-link--no-visited-state" href={}>Sign out</a>
and click the link again to accept this invite.
invite-team-members
2018-02-19 16:53:29 +00:00
""".format(
current_user.email_address,
Stop using `_external=True` in tests It looks like, by default, Flask no longer makes full URLs, for example `https://example.com/path`. Instead it does `/path`. This will still work fine, and if anything is better because it reduces the number of bytes of HTML we are sending. It won’t mean that requests go over `http` instead of `https` without the protocol because we set the appropriate HSTS header here: https://github.com/alphagov/notifications-aws/blob/0c57da7781260c56575b796062795a9d8e1f8c87/ansible/roles/paas-proxy/templates/admin.conf.j2#L11 This commit changes all our tests to reflect that URLs no longer have the protocol and domain in them. `_external=True` is Flask’s way of saying whether a URL should be generated with the domain and protocol (`True`) or without it (`False`). Again, I can’t find the changelog or diff where this was introuduced, but if you’d like to go spelunking then here’s a starting point: https://github.com/pallets/flask/blob/50374e3cfe815acd24fd390c861561a0dd17bfbb/src/flask/helpers.py#L192
2022-05-27 14:46:35 +01:00
url_for("main.sign_out")))
invite-team-members
2018-02-19 16:53:29 +00:00
flash(message=message)
abort(403)
if invited_org_user.status == 'cancelled':
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
organisation = Organisation.from_id(invited_org_user.organisation)
invite-team-members
2018-02-19 16:53:29 +00:00
return render_template('views/cancelled-invitation.html',
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
from_user=invited_org_user.invited_by.name,
organisation_name=organisation.name)
invite-team-members
2018-02-19 16:53:29 +00:00
if invited_org_user.status == 'accepted':
store invited org user ids in session first of a two step process to remove invited user objects from the session. we're removing them because they're of variable size, and with a lot of folder permissions they can cause the session to exceed the 4kb cookie size limit and not save properly. this commit looks at invited org users only. in this step, start saving the invited org user's id to the session alongside the session object. Then, if the invited_org_user_id is present in the next step of the invite flow, fetch the user object from the API instead of from the session. If it's not present (due to a session set by an older instance of the admin app), then just use the old code to get the entire object out of the session. For invites where the user is small enough to persist to the cookie, this will still save both the old and the new way, but will always make an extra check to the API, I think this minor performance hit is totally fine. For invites where the user is too big to persist, they'll still fail for now, and will need to wait until the next PR comes along and stops saving the large invited user object to the session entirely.
2021-03-08 10:01:12 +00:00
session.pop('invited_org_user_id', None)
invite-team-members
2018-02-19 16:53:29 +00:00
return redirect(url_for('main.organisation_dashboard', org_id=invited_org_user.organisation))
store invited org user ids in session first of a two step process to remove invited user objects from the session. we're removing them because they're of variable size, and with a lot of folder permissions they can cause the session to exceed the 4kb cookie size limit and not save properly. this commit looks at invited org users only. in this step, start saving the invited org user's id to the session alongside the session object. Then, if the invited_org_user_id is present in the next step of the invite flow, fetch the user object from the API instead of from the session. If it's not present (due to a session set by an older instance of the admin app), then just use the old code to get the entire object out of the session. For invites where the user is small enough to persist to the cookie, this will still save both the old and the new way, but will always make an extra check to the API, I think this minor performance hit is totally fine. For invites where the user is too big to persist, they'll still fail for now, and will need to wait until the next PR comes along and stops saving the large invited user object to the session entirely.
2021-03-08 10:01:12 +00:00
session['invited_org_user_id'] = invited_org_user.id
invite-team-members
2018-02-19 16:53:29 +00:00
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
existing_user = User.from_email_address_or_none(invited_org_user.email_address)
organisation_users = OrganisationUsers(invited_org_user.organisation)
invite-team-members
2018-02-19 16:53:29 +00:00
if existing_user:
Update `email_access_validated_at` on invite Accepting an invite means that you’ve just clicked a link in your email inbox. This shows that you have access to your email. We can make a record of this, thereby extending the time before we ask you to revalidate your email address.
2021-08-17 16:38:37 +01:00
existing_user.update_email_access_validated_at()
Make user API client return JSON, not a model The data flow of other bits of our application looks like this: ``` API (returns JSON) ⬇ API client (returns a built in type, usually `dict`) ⬇ Model (returns an instance, eg of type `Service`) ⬇ View (returns HTML) ``` The user API client was architected weirdly, in that it returned a model directly, like this: ``` API (returns JSON) ⬇ API client (returns a model, of type `User`, `InvitedUser`, etc) ⬇ View (returns HTML) ``` This mixing of different layers of the application is bad because it makes it hard to write model code that doesn’t have circular dependencies. As our application gets more complicated we will be relying more on models to manage this complexity, so we should make it easy, not hard to write them. It also means that most of our mocking was of the User model, not just the underlying JSON. So it would have been easy to introduce subtle bugs to the user model, because it wasn’t being comprehensively tested. A lot of the changed lines of code in this commit mean changing the tests to mock only the JSON, which means that the model layer gets implicitly tested. For those reasons this commit changes the user API client to return JSON, not an instance of `User` or other models.
2019-05-23 15:27:35 +01:00
invited_org_user.accept_invite()
invite-team-members
2018-02-19 16:53:29 +00:00
if existing_user not in organisation_users:
Refactor model to put `add_to…` methods on user An invited user can’t be added to an organisation or service, only a real user can. So the methods to do this should be on the user model, and take the details of the invite as arguments.
2019-06-27 15:48:29 +01:00
existing_user.add_to_organisation(organisation_id=invited_org_user.organisation)
invite-team-members
2018-02-19 16:53:29 +00:00
return redirect(url_for('main.organisation_dashboard', org_id=invited_org_user.organisation))
else:
return redirect(url_for('main.register_from_org_invite'))
Reference in New Issue Copy Permalink