This includes a fix for the commit hash test, which was not referencing the Flask app in a valid way.
Signed-off-by: Carlo Costino <carlo.costino@gsa.gov>
Note that this commit has some failing tests with it that also needed to be fixed; it is unclear why they are failing at the moment, though.
Signed-off-by: Carlo Costino <carlo.costino@gsa.gov>
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:
0c57da7781/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:
50374e3cfe/src/flask/helpers.py (L192)
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
Lots of our tests still use an older fixture called `client`. This is
not as good because it:
- returns a raw `Response` object
- doesn’t do the additional checks
- means our tests contain a lot of repetetive boilerplate like `page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')`
This commit converts all the tests which had a `client.get(…)` or
`client.post(…)` statement to use their equivalents on `client_request`
instead.
Subsequent commits will remove uses of `client` in other tests, but
doing it this way means the work can be broken up into more manageable
chunks.
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.
This is useful information to store for the event, which would be
lost if someone subsequently changed them.
Rather than updating lots of mock assertions, I've replaced them
with a single test / assert at a lower level, which is consistent
with auditing being a non-critical function.
Usually we have imports at the top. It looks like the reason for
them being inline was to avoid a circular import, but we can also
avoid this by not importing everything from the app module.
Since we're about to add more imports from event_handlers, now is
a good time to refactor them. Note this matches how we import the
event handlers in every other module.
Previously we made surprising changed to the invited user as part
of the mock, and then surprising assertions that its ID matched
USER_ONE_ID. This simplifies the mock to do what it says, so that
we can test for the original ID of the existing user.*
*this does still differ from the ID of the sample_invite, which is
also hard-coded to USER_ONE_ID. However, this isn't relevant in
any of the tests, so doesn't seem to much of an issue.
This replaces the original fixture with a more explicit one, noting
that none of the tests rely on this fixture as part of testing the
scenarios when a user is already a member of the service.
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.
At the moment if you’re invited to a live broadcast service you get the
training mode tour. This is misleading, and could make people think they
weren’t in danger of sending a real alert.
This commit adds a short, 2 step tour for users invited to a live
broadcast service.
For someone who has retrieved a template ID from their system the only
way to find it in Notify is:
- hack the URL
- click through every template, visually inspecting the ID shown on the
page until you find the right one
Neither of these is ideal.
This commit adds searching by ID, for those services who have an API
integration. This means we don’t need to confuse teams who aren’t using
the API by talking about IDs.
This is similar to how we let these teams search for notifications by
reference[1]
1. https://github.com/alphagov/notifications-admin/pull/3223/files
now that we no longer set it since
https://github.com/alphagov/notifications-admin/pull/3841 was merged, we
don't need to remove it either. And we can remove checks that expect it
when cleaning up the session. And the unit tests that make sure we
ignore it if it's in the session.
So long, session['invited_user'] and session['invited_org_user']!
the invited_user objects can be arbitrarily large, and when we put them
in the session we risk going over the session cookie's 4kb size limit.
since https://github.com/alphagov/notifications-admin/pull/3827 was
merged, we store the user id in the session. Now that's been live for a
day or two we can safely stop putting the rich object in the session.
Needed to change a bunch of tests for this to make sure appropriate
mocks were set. Also some tests were accidentally re-using fake_uuid.
Still pop the object when cleaning up sessions. We'll need to remove
that in a future PR.
API gives an error if it tries to add a user to a service and that user is
already a member of the service. This situation shouldn't occur - admin checks
if an invited user is a member of a service before calling API, but we
have seen this error occurring when there are two requests processing at
the same time.
This change catches the errors from API if a user is already a member of
a service and redirects the user to the service dashboard so that they
don't see an error page.
